fix: Fix handling of Symbol and non-enumerable properties in finalization / freeze. Fixes #1096, #1087, #1091 (#1105))

Author: mweststrate

__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) {

__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", () => {

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)
 	}
 }

src/utils/common.ts

@@ -60,15 +60,19 @@ export function original(value: Drafted<any>): 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<T extends Objectish>(
 	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<T>(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
 }