Skip to content

fix: preserve enumerable symbols when cloning patch values - #1354

Open
wozhenbang2004 wants to merge 4 commits into
immerjs:mainfrom
wozhenbang2004:patch-1
Open

wozhenbang2004 wants to merge 4 commits into
immerjs:mainfrom
wozhenbang2004:patch-1

Conversation

@wozhenbang2004

Copy link
Copy Markdown

Applying an object patch value drops its enumerable symbol properties because deepClonePatchValue only iterates string keys. A value produced by produceWithPatches can therefore differ from replaying its patches.

const key = Symbol("key")
const [next, patches] = produceWithPatches({}, draft => {
  draft.item = {[key]: {value: 1}}
})
const replayed = applyPatches({}, patches)
// next.item[key] is {value: 1}; replayed.item[key] is currently undefined

Clone own enumerable symbols recursively alongside the existing string-key handling. Non-enumerable symbols remain excluded, and the existing class prototype and immerable handling are preserved.

Tests cover add, replace, inverse patches, and nested symbol-value clone isolation.

Validation: source suite and coverage (4,215 passed, 8 skipped), production-build suite (3,664 passed, 8 skipped), build including declarations, Flow (0 errors), performance suite, Prettier, and diff checks.

Deep-clone own enumerable symbol properties so applying object patch values retains their data.
Cover add, replace, inverse patches, and deep clone isolation for nested enumerable symbol properties.

@mweststrate mweststrate left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Probably the fix should respect the setUseStrictShallowCopy flag for consistent behavior with normal recipes.

Comment thread src/plugins/patches.ts Outdated
if (isSet(obj)) return new Set(Array.from(obj).map(deepClonePatchValue))
const cloned = Object.create(getPrototypeOf(obj))
for (const key in obj) cloned[key] = deepClonePatchValue(obj[key])
for (const key of Object.getOwnPropertySymbols(obj)) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The main package iteration is determined by the flag setUseStrictShallowCopy . Please make sure the code here is consistent with that. Probably easiest to prove by rewriting the test to use the test helper runPatchTests with the option enabled and disabled.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants