Skip to content

fix: preserve assignments that shadow inherited values - #1305

Open
jakezwang wants to merge 1 commit into
immerjs:mainfrom
jakezwang:fix-inherited-value-assignment
Open

jakezwang wants to merge 1 commit into
immerjs:mainfrom
jakezwang:fix-inherited-value-assignment

Conversation

@jakezwang

Copy link
Copy Markdown

Assigning a prototype's existing value to a draft should create an own property. Currently, draft.name = "default" is ignored when name: "default" is inherited; the result has no own name and no corresponding patch.

Only skip equal-value assignments when the property is already owned by the base or copy. This also covers inherited NaN values and drafts that were modified earlier in the recipe.

Tests check property ownership, prototype preservation, and forward/inverse patches. Validation: yarn test, yarn coverage, and yarn test:perf on Node 24.21.0; the same checks also passed on Node 22.23.2.

@mweststrate mweststrate added the can-merge-if-someone-actually-cares Valid fix, but unclear cost/benefit and may hinder future work. Can merge if someone champions it. label Oct 21, 2026
@mweststrate

Copy link
Copy Markdown
Collaborator

Marked mergable per #1335 (comment). Might be good to have some rationale why this is important to have; not allocating that field could also be considered a feature?

@jakezwang

Copy link
Copy Markdown
Author

My rationale is consistency with #1160: assigning an inherited undefined already creates an own property and an add patch. This extends that behavior to equal inherited values such as "default" and NaN.

For Object.create({[immerable]: true, name: "default"}), an explicit draft.name = "default" currently leaves Object.keys(result) empty and JSON.stringify(result) as {}. The proposed behavior includes name / {"name":"default"} and records the override as an add patch with inverse remove. Reading result.name alone hides that distinction. Existing equal writes to an own property still take the no-op path.

This was an independently found edge case; I do not have a production use case to add. I understand the cost and future-implementation concern: it adds ownership checks to assignment. Whether this distinction belongs in Immer's contract is an API policy decision. If a concrete consumer is needed to justify that cost, this can remain deferred until one exists.

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

can-merge-if-someone-actually-cares Valid fix, but unclear cost/benefit and may hinder future work. Can merge if someone champions it.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants