Prevent stale shape data in render (#3882)

Our Shape component is set up to, by default, not re-render the actual
shape content when the shape's x,y coords or opacity or rotation etc
change, since those things don't affect the shape itself but rather how
it is composited. It does this by only triggering re-renders when
shape.props or shape.meta change using react.memo.

However the shape's render is also reactive so it is possible to trigger
re-renders even when shape.props and shape.meta do not change, e.g. in
the case of arrow shapes you can trigger re-renders by updating bindings
involving the arrow, or by moving one of the arrow's bound shapes.

This is fine except that the actual arrow record being passed into the
util.component etc methods was not always the very latest version in the
store because it has been memoized by react. This makes identity checks
like `shape === otherShape` fail sometimes, and was causing a bug in
arrow rendering (the grey bits to show the binding anchor were not
always showing up).

To fix that, this PR simply plucks the shape out of the store using the
sneaky non-capturing method during render so that it's always up-to-date
when being passed to the shape util for rendering, while still
preserving the behaviour that by default it won't re-render unless the
shape.props or shape.meta change.

### Change Type

<!--  Please select a 'Scope' label ️ -->

- [x] `sdk` — Changes the tldraw SDK
- [ ] `dotcom` — Changes the tldraw.com web app
- [ ] `docs` — Changes to the documentation, examples, or templates.
- [ ] `vs code` — Changes to the vscode plugin
- [ ] `internal` — Does not affect user-facing stuff

<!--  Please select a 'Type' label ️ -->

- [x] `bugfix` — Bug fix
- [ ] `feature` — New feature
- [ ] `improvement` — Improving existing features
- [ ] `chore` — Updating dependencies, other boring stuff
- [ ] `galaxy brain` — Architectural changes
- [ ] `tests` — Changes to any test code
- [ ] `tools` — Changes to infrastructure, CI, internal scripts,
debugging tools, etc.
- [ ] `dunno` — I don't know


### Test Plan

1. Add a step-by-step description of how to test your PR here.
2.

- [ ] Unit Tests
- [ ] End to end tests

### Release Notes

- Add a brief release note for your PR here.
This commit is contained in:
David Sheldrick 2024-06-05 11:48:52 +01:00 committed by GitHub
parent 9104e6abf8
commit b04ded47c3
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 15 additions and 3 deletions

View file

@ -175,7 +175,11 @@ export const Shape = memo(function Shape({
const InnerShape = memo( const InnerShape = memo(
function InnerShape<T extends TLShape>({ shape, util }: { shape: T; util: ShapeUtil<T> }) { function InnerShape<T extends TLShape>({ shape, util }: { shape: T; util: ShapeUtil<T> }) {
return useStateTracking('InnerShape:' + shape.type, () => util.component(shape)) return useStateTracking('InnerShape:' + shape.type, () =>
// always fetch the latest shape from the store even if the props/meta have not changed, to avoid
// calling the render method with stale data.
util.component(util.editor.store.unsafeGetWithoutCapture(shape.id) as T)
)
}, },
(prev, next) => prev.shape.props === next.shape.props && prev.shape.meta === next.shape.meta (prev, next) => prev.shape.props === next.shape.props && prev.shape.meta === next.shape.meta
) )
@ -188,7 +192,11 @@ const InnerShapeBackground = memo(
shape: T shape: T
util: ShapeUtil<T> util: ShapeUtil<T>
}) { }) {
return useStateTracking('InnerShape:' + shape.type, () => util.backgroundComponent?.(shape)) return useStateTracking('InnerShape:' + shape.type, () =>
// always fetch the latest shape from the store even if the props/meta have not changed, to avoid
// calling the render method with stale data.
util.backgroundComponent?.(util.editor.store.unsafeGetWithoutCapture(shape.id) as T)
)
}, },
(prev, next) => prev.shape.props === next.shape.props && prev.shape.meta === next.shape.meta (prev, next) => prev.shape.props === next.shape.props && prev.shape.meta === next.shape.meta
) )

View file

@ -10,7 +10,11 @@ import { OptionalErrorBoundary } from '../ErrorBoundary'
// need an extra layer of indirection here to allow hooks to be used inside the indicator render // need an extra layer of indirection here to allow hooks to be used inside the indicator render
const EvenInnererIndicator = ({ shape, util }: { shape: TLShape; util: ShapeUtil<any> }) => { const EvenInnererIndicator = ({ shape, util }: { shape: TLShape; util: ShapeUtil<any> }) => {
return useStateTracking('Indicator: ' + shape.type, () => util.indicator(shape)) return useStateTracking('Indicator: ' + shape.type, () =>
// always fetch the latest shape from the store even if the props/meta have not changed, to avoid
// calling the render method with stale data.
util.indicator(util.editor.store.unsafeGetWithoutCapture(shape.id) as TLShape)
)
} }
const InnerIndicator = ({ editor, id }: { editor: Editor; id: TLShapeId }) => { const InnerIndicator = ({ editor, id }: { editor: Editor; id: TLShapeId }) => {