From c0e6749e8df3e20d880d61b726b1395167ba2088 Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Tue, 20 Oct 2020 21:34:40 +0100 Subject: [PATCH 1/2] fix: reconcile if the original value is assigned after creating a draft. Fixes #659 --- __tests__/regressions.js | 81 ++++++++++++++++++++++++++++++++++++++++ src/core/proxy.ts | 22 ++++++++--- 2 files changed, 97 insertions(+), 6 deletions(-) diff --git a/__tests__/regressions.js b/__tests__/regressions.js index 9ae9138b..d3e63cac 100644 --- a/__tests__/regressions.js +++ b/__tests__/regressions.js @@ -158,5 +158,86 @@ function runBaseTest(name, useProxies, autoFreeze, useListener) { }) expect(newData.foo[0].isActive).toBe(true) }) + + test("#659 no reconciliation after read", () => { + const bar = {} + const foo = {bar} + + const next = produce(foo, draft => { + draft.bar + draft.bar = bar + }) + expect(next).toBe(foo) + }) + + test("#659 no reconciliation after read - 2", () => { + const bar = {} + const foo = {bar} + + const next = produce(foo, draft => { + const subDraft = draft.bar + draft.bar = bar + subDraft.x = 3 // this subDraft is not part of the end result, so ignore + }) + + expect(next).toEqual(foo) + }) + + test("#659 no reconciliation after read - 3", () => { + const bar = {} + const foo = {bar} + + const next = produce(foo, draft => { + const subDraft = draft.bar + subDraft.x = 3 // this subDraft is not part of the end result, so ignore + draft.bar = bar + }) + expect(next).toEqual(foo) + }) + + // Disabled: these are optimizations that would be nice if they + // could be detected, but don't change the correctness of the result + test.skip("#659 no reconciliation after read - 4", () => { + const bar = {} + const foo = {bar} + + const next = produce(foo, draft => { + const subDraft = draft.bar + draft.bar = bar + subDraft.x = 3 // this subDraft is not part of the end result, so ignore + }) + + expect(next).toBe(foo) + }) + + // Disabled: these are optimizations that would be nice if they + // could be detected, but don't change the correctness of the result + test.skip("#659 no reconciliation after read - 5", () => { + const bar = {} + const foo = {bar} + + const next = produce(foo, draft => { + const subDraft = draft.bar + subDraft.x = 3 // this subDraft is not part of the end result, so ignore + draft.bar = bar + }) + expect(next).toBe(foo) + }) + + test("#659 no reconciliation after read - 6", () => { + const bar = {} + const foo = {bar} + + const next = produce(foo, draft => { + const subDraft = draft.bar + subDraft.x = 3 // this subDraft is not part of the end result, so ignore + draft.bar = bar + draft.bar = subDraft + }) + expect(next).not.toBe(foo) + expect(next).toEqual({ + bar: {x: 3} + }) + }) }) } diff --git a/src/core/proxy.ts b/src/core/proxy.ts index 13140a5c..dbccbc20 100644 --- a/src/core/proxy.ts +++ b/src/core/proxy.ts @@ -18,6 +18,7 @@ import { ProxyTypeProxyObject, ProxyTypeProxyArray } from "../internal" +import {isDraft} from "../utils/common" interface ProxyBaseState extends ImmerBaseState { assigned_: { @@ -129,7 +130,11 @@ export const objectTraps: ProxyHandler = { ownKeys(state) { return Reflect.ownKeys(latest(state)) }, - set(state, prop: string /* strictly not, but helps TS */, value) { + set( + state: ProxyObjectState, + prop: string /* strictly not, but helps TS */, + value + ) { const desc = getDescriptorFromProto(latest(state), prop) if (desc?.set) { // special case: if this write is captured by a setter, we have @@ -137,20 +142,25 @@ export const objectTraps: ProxyHandler = { desc.set.call(state.draft_, value) return true } - state.assigned_[prop] = true if (!state.modified_) { // the last check is because we need to be able to distinguish setting a non-existig to undefined (which is a change) // from setting an existing property with value undefined to undefined (which is not a change) - if ( - is(value, peek(latest(state), prop)) && - (value !== undefined || has(state.base_, prop)) - ) + const current = peek(latest(state), prop) + // special case, if we assigning the original value to a draft, we can ignore the assignment + const currentState: ProxyObjectState = current?.[DRAFT_STATE] + if (currentState && currentState.base_ === value) { + state.copy_![prop] = value + state.assigned_[prop] = false + return true + } + if (is(value, current) && (value !== undefined || has(state.base_, prop))) return true prepareCopy(state) markChanged(state) } // @ts-ignore state.copy_![prop] = value + state.assigned_[prop] = true return true }, deleteProperty(state, prop: string) { From 41673f49f2b2e4d8df9e173bc8792996e26987ae Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Tue, 20 Oct 2020 21:42:48 +0100 Subject: [PATCH 2/2] docs: clarify that patches are not optimal. Solves #648 --- docs/patches.md | 2 ++ docs/pitfalls.md | 1 + 2 files changed, 3 insertions(+) diff --git a/docs/patches.md b/docs/patches.md index b3d525c7..ed90add8 100644 --- a/docs/patches.md +++ b/docs/patches.md @@ -112,6 +112,8 @@ The generated patches are similar (but not the same) to the [RFC-6902 JSON patch ] ``` +⚠ Note: The set of patches generated by Immer should be correct, that is, applying them to an equal base object should result in the same end state. However Immer does not guarantee the generated set of patches will be optimal, that is, the minimum set of patches possible. It depends often on the use case what is considered 'optimal', and generating the optimal set of patches is potentielly computationally very expensive. So in cases you might want to post process the generated patches, or compress them as explained below. + ### `produceWithPatches`
diff --git a/docs/pitfalls.md b/docs/pitfalls.md index 0adefcae..480b20b0 100644 --- a/docs/pitfalls.md +++ b/docs/pitfalls.md @@ -16,6 +16,7 @@ title: Pitfalls 1. You will need to enable your own classes to work properly with Immer. For docs on the topic, check out the section on [working with complex objects](https://immerjs.github.io/immer/docs/complex-objects). 1. For arrays, only numeric properties and the `length` property can be mutated. Custom properties are not preserved on arrays. 1. Note that data that comes from the closure, and not from the base state, will never be drafted, even when the data has become part of the new draft: +1. The set of patches generated by Immer should be correct, that is, applying them to an equal base object should result in the same end state. However Immer does not guarantee the generated set of patches will be optimal, that is, the minimum set of patches possible. ```javascript function onReceiveTodo(todo) {