From 8949a3ee68f187573e61f5345f2b8b59bcd0813c Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Sat, 9 Mar 2024 22:28:39 +0100 Subject: [PATCH] fix: Fix handling of Symbol and non-enumerable properties in finalization / freeze. Fixes #1096, #1087, #1091 (#1105)) --- __tests__/base.js | 16 ++++++++++++++++ __tests__/frozen.js | 1 + src/core/finalize.ts | 17 ++++++++++------- src/utils/common.ts | 17 ++++++++++++----- 4 files changed, 39 insertions(+), 12 deletions(-) diff --git a/__tests__/base.js b/__tests__/base.js index 3e900bb5..1c0059aa 100644 --- a/__tests__/base.js +++ b/__tests__/base.js @@ -2362,6 +2362,22 @@ function testObjectTypes(produce) { [customSymbol]: 3 }) }) + + describe("#1096 / #1087 / proxies", () => { + const sym = Symbol() + + let state = { + id: 1, + [sym]: {x: 2} + } + + state = produce(state, draft => { + draft.id = 2 + draft[sym] + }) + + expect(state[sym].x).toBe(2) + }) } function testLiteralTypes(produce) { diff --git a/__tests__/frozen.js b/__tests__/frozen.js index b0080f54..f130f3c2 100644 --- a/__tests__/frozen.js +++ b/__tests__/frozen.js @@ -219,6 +219,7 @@ function runTests(name) { state2.ref.state.x++ }).not.toThrow() expect(state2.ref.state.x).toBe(2) + expect(component.state.x).toBe(2) }) it("never freezes symbolic fields #590", () => { diff --git a/src/core/finalize.ts b/src/core/finalize.ts index 7c446316..6ee69ce6 100644 --- a/src/core/finalize.ts +++ b/src/core/finalize.ts @@ -58,11 +58,8 @@ function finalize(rootScope: ImmerScope, value: any, path?: PatchPath) { const state: ImmerState = value[DRAFT_STATE] // A plain object, might need freezing, might contain drafts if (!state) { - each( - value, - (key, childValue) => - finalizeProperty(rootScope, state, value, key, childValue, path), - true // See #590, don't recurse into non-enumerable of non drafted objects + each(value, (key, childValue) => + finalizeProperty(rootScope, state, value, key, childValue, path) ) return value } @@ -148,8 +145,14 @@ function finalizeProperty( return } finalize(rootScope, childValue) - // immer deep freezes plain objects, so if there is no parent state, we freeze as well - if (!parentState || !parentState.scope_.parent_) + // Immer deep freezes plain objects, so if there is no parent state, we freeze as well + // Per #590, we never freeze symbolic properties. Just to make sure don't accidentally interfere + // with other frameworks. + if ( + (!parentState || !parentState.scope_.parent_) && + typeof prop !== "symbol" && + Object.prototype.propertyIsEnumerable.call(targetObject, prop) + ) maybeFreeze(rootScope, childValue) } } diff --git a/src/utils/common.ts b/src/utils/common.ts index 1ebb5b5f..fb4a768d 100644 --- a/src/utils/common.ts +++ b/src/utils/common.ts @@ -60,15 +60,19 @@ export function original(value: Drafted): any { return value[DRAFT_STATE].base_ } +/** + * Each iterates a map, set or array. + * Or, if any other kind of of object all it's own properties. + * Regardless whether they are enumerable or symbols + */ export function each( obj: T, - iter: (key: string | number, value: any, source: T) => void, - enumerableOnly?: boolean + iter: (key: string | number, value: any, source: T) => void ): void export function each(obj: any, iter: any) { if (getArchtype(obj) === ArchType.Object) { - Object.entries(obj).forEach(([key, value]) => { - iter(key, value, obj) + Reflect.ownKeys(obj).forEach(key => { + iter(key, obj[key], obj) }) } else { obj.forEach((entry: any, index: any) => iter(index, entry, obj)) @@ -191,7 +195,10 @@ export function freeze(obj: any, deep: boolean = false): T { obj.set = obj.add = obj.clear = obj.delete = dontMutateFrozenCollections as any } Object.freeze(obj) - if (deep) each(obj, (_key, value) => freeze(value, true), true) + if (deep) + // See #590, don't recurse into non-enumerable / Symbol properties when freezing + // So use Object.entries (only string-like, enumerables) instead of each() + Object.entries(obj).forEach(([key, value]) => freeze(value, true)) return obj }