Skip to content

Commit

Permalink
fix stale closure in InnerShape (tldraw#5117)
Browse files Browse the repository at this point in the history
fixes tldraw#5047

the problem was we weren't destroying the shape effect schedulers when
the editor changed, because they were only keyed by the shape type name,
which obviously does not change when the editor is reinstantiated.

this augments useStateTracking to accept a deps array, and uses the
identity of the shape util as part of the deps array.

### Change type

- [x] `bugfix`
  • Loading branch information
ds300 authored Dec 13, 2024
1 parent f573a67 commit b268224
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 13 deletions.
32 changes: 22 additions & 10 deletions packages/editor/src/lib/components/Shape.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -171,13 +171,19 @@ export const Shape = memo(function Shape({

export const InnerShape = memo(
function InnerShape<T extends TLShape>({ shape, util }: { shape: T; util: ShapeUtil<T> }) {
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)
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),
[util, shape.id]
)
},
(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 &&
prev.util === next.util
)

export const InnerShapeBackground = memo(
Expand All @@ -188,11 +194,17 @@ export const InnerShapeBackground = memo(
shape: T
util: ShapeUtil<T>
}) {
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)
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),
[util, shape.id]
)
},
(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 &&
prev.util === next.util
)
2 changes: 1 addition & 1 deletion packages/state-react/api-report.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export function useQuickReactor(name: string, reactFn: () => void, deps?: any[])
export function useReactor(name: string, reactFn: () => void, deps?: any[] | undefined): void;

// @public
export function useStateTracking<T>(name: string, render: () => T): T;
export function useStateTracking<T>(name: string, render: () => T, deps?: unknown[]): T;

// @public
export function useValue<Value>(value: Signal<Value>): Value;
Expand Down
5 changes: 3 additions & 2 deletions packages/state-react/src/lib/useStateTracking.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import React from 'react'
*
* @public
*/
export function useStateTracking<T>(name: string, render: () => T): T {
export function useStateTracking<T>(name: string, render: () => T, deps: unknown[] = []): T {
// This hook creates an effect scheduler that will trigger re-renders when its reactive dependencies change, but it
// defers the actual execution of the effect to the consumer of this hook.

Expand Down Expand Up @@ -56,7 +56,8 @@ export function useStateTracking<T>(name: string, render: () => T): T {
const getSnapshot = () => scheduler.scheduleCount

return [scheduler, subscribe, getSnapshot]
}, [name])
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [name, ...deps])

React.useSyncExternalStore(subscribe, getSnapshot, getSnapshot)

Expand Down

0 comments on commit b268224

Please sign in to comment.