Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: frozen afterward with targetCache #71

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 13 additions & 9 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -206,18 +206,22 @@ export const createProxy = <T>(
targetCache?: WeakMap<object, unknown>,
): T => {
if (!isObjectToTrack(obj)) return obj;
let targetAndCopied =
targetCache && (targetCache as TargetCache<typeof obj>).get(obj);
if (!targetAndCopied) {
const target = getOriginalObject(obj);
if (needsToCopyTargetObject(target)) {
Comment on lines -212 to -213
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, previously, we can avoid these computations with the target cache.
I think getOriginalObject is fairly fast, but needsToCopyTargetObject has a loop.

So, there's a performance drawback. Since the target cache is introduced to improve the performance in Valtio, it has to be considered.

targetAndCopied = [target, copyTargetObject(target)];

const target = getOriginalObject(obj);
let copiedTarget;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let copiedTarget;
let copiedTarget: typeof target | undefined;


if (needsToCopyTargetObject(target)) {
const targetAndCopied =
targetCache && (targetCache as TargetCache<typeof obj>).get(obj);
Comment on lines +214 to +215
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this usage, we don't need to store the target in the cache.


if (targetAndCopied) {
copiedTarget = targetAndCopied[1];
} else {
targetAndCopied = [target];
copiedTarget = copyTargetObject(target);
targetCache?.set(obj, [target, copiedTarget]);
}
targetCache?.set(obj, targetAndCopied);
}
const [target, copiedTarget] = targetAndCopied;

let handlerAndState =
proxyCache && (proxyCache as ProxyCache<typeof target>).get(target);
if (
Expand Down
26 changes: 26 additions & 0 deletions tests/20_immer_v8.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,30 @@ describe('immer v8', () => {
expect(isChanged(s1, { a: { b: 'b' } }, a2)).toBe(false);
expect(isChanged(s1, { a: { b: 'b2' } }, a2)).toBe(true);
});

// See: https://github.com/dai-shi/react-tracked/issues/70
it('should work if object is frozen afterward using targetCache', () => {
const targetCache = new WeakMap();

const s1 = { a: { b: 'b' } };
const a1 = new WeakMap();
const p1 = createProxy(s1, a1, undefined, targetCache);
noop(p1.a);

expect(isChanged(s1, s1, a1)).toBe(false);
expect(isChanged(s1, { a: s1.a }, a1)).toBe(false);
expect(isChanged(s1, { a: { b: 'b' } }, a1)).toBe(true);

Object.freeze(s1.a);
Object.freeze(s1);

const a2 = new WeakMap();
const p2 = createProxy(s1, a2, undefined, targetCache);
noop(p2.a.b);

expect(isChanged(s1, s1, a2)).toBe(false);
expect(isChanged(s1, { a: s1.a }, a2)).toBe(false);
expect(isChanged(s1, { a: { b: 'b' } }, a2)).toBe(false);
expect(isChanged(s1, { a: { b: 'b2' } }, a2)).toBe(true);
});
});
Loading