From 8aa4da345fe7810740036f1140f21ee362f6ddf5 Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Wed, 11 Sep 2019 01:31:11 +0200 Subject: [PATCH 1/5] updated tests --- __tests__/frozen.js | 83 ++++++++++++++++++++++++--------------------- 1 file changed, 44 insertions(+), 39 deletions(-) diff --git a/__tests__/frozen.js b/__tests__/frozen.js index 2c690430..a146c705 100644 --- a/__tests__/frozen.js +++ b/__tests__/frozen.js @@ -50,72 +50,77 @@ function runTests(name, useProxies) { expect(isFrozen(next)).toBeTruthy() expect(isFrozen(next.a)).toBeTruthy() }) - }) - - describe("the result is never auto-frozen when", () => { - it("the producer is a no-op", () => { - const base = {} - const next = produce(base, () => {}) - expect(next).toBe(base) - expect(isFrozen(next)).toBeFalsy() - }) - - it("the root draft is returned", () => { - const base = {} - const next = produce(base, draft => draft) - expect(next).toBe(base) - expect(isFrozen(next)).toBeFalsy() - }) - - it("a nested draft is returned", () => { - const base = {a: {}} - const next = produce(base, draft => draft.a) - expect(next).toBe(base.a) - expect(isFrozen(next)).toBeFalsy() - }) - - it("the base state is returned", () => { - const base = {} - const next = produce(base, () => base) - expect(next).toBe(base) - expect(isFrozen(next)).toBeFalsy() - }) it("a new object replaces a primitive base", () => { - const obj = {} + const obj = {a: {}} const next = produce(null, () => obj) expect(next).toBe(obj) - expect(isFrozen(next)).toBeFalsy() + expect(isFrozen(next)).toBeTruthy() + expect(isFrozen(next.a)).toBeTruthy() }) it("a new object replaces the entire draft", () => { const obj = {a: {b: {}}} const next = produce({}, () => obj) expect(next).toBe(obj) - expect(isFrozen(next)).toBeFalsy() - expect(isFrozen(next.a)).toBeFalsy() - expect(isFrozen(next.a.b)).toBeFalsy() + expect(isFrozen(next)).toBeTruthy() + expect(isFrozen(next.a)).toBeTruthy() + expect(isFrozen(next.a.b)).toBeTruthy() }) it("a new object is added to the root draft", () => { const base = {} const next = produce(base, draft => { - draft.a = {} + draft.a = {b: []} }) expect(next).not.toBe(base) expect(isFrozen(next)).toBeTruthy() - expect(isFrozen(next.a)).toBeFalsy() + expect(isFrozen(next.a)).toBeTruthy() + expect(isFrozen(next.b)).toBeTruthy() }) it("a new object is added to a nested draft", () => { const base = {a: {}} const next = produce(base, draft => { - draft.a.b = {} + draft.a.b = {c: {}} }) expect(next).not.toBe(base) expect(isFrozen(next)).toBeTruthy() expect(isFrozen(next.a)).toBeTruthy() - expect(isFrozen(next.a.b)).toBeFalsy() + expect(isFrozen(next.a.b)).toBeTruthy() + expect(isFrozen(next.a.b.c)).toBeTruthy() + }) + }) + + describe("the result is never auto-frozen when", () => { + it("the producer is a no-op", () => { + const base = {a: {}} + const next = produce(base, () => {}) + expect(next).toBe(base) + expect(isFrozen(next)).toBeFalsy() + expect(isFrozen(next.a)).toBeFalsy() + }) + + it("the root draft is returned", () => { + const base = {a: {}} + const next = produce(base, draft => draft) + expect(next).toBe(base) + expect(isFrozen(next)).toBeFalsy() + expect(isFrozen(next.a)).toBeFalsy() + }) + + it("a nested draft is returned", () => { + const base = {a: {}} + const next = produce(base, draft => draft.a) + expect(next).toBe(base.a) + expect(isFrozen(next)).toBeFalsy() + }) + + it("the base state is returned", () => { + const base = {} + const next = produce(base, () => base) + expect(next).toBe(base) + expect(isFrozen(next)).toBeFalsy() }) }) From 7fd3ef38cf4047eb33a51a897e9ec1af13f36cfc Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Wed, 11 Sep 2019 21:53:31 +0200 Subject: [PATCH 2/5] Implemented deepFreeze functionality, *anything draftable* returned from a produce will now be deeply frozen --- __tests__/frozen.js | 50 ++++++++++++++++++++++----------------------- readme.md | 2 ++ src/common.js | 9 ++++++++ src/immer.js | 19 ++++++++++++++--- 4 files changed, 51 insertions(+), 29 deletions(-) diff --git a/__tests__/frozen.js b/__tests__/frozen.js index a146c705..e1c3df8a 100644 --- a/__tests__/frozen.js +++ b/__tests__/frozen.js @@ -51,14 +51,6 @@ function runTests(name, useProxies) { expect(isFrozen(next.a)).toBeTruthy() }) - it("a new object replaces a primitive base", () => { - const obj = {a: {}} - const next = produce(null, () => obj) - expect(next).toBe(obj) - expect(isFrozen(next)).toBeTruthy() - expect(isFrozen(next.a)).toBeTruthy() - }) - it("a new object replaces the entire draft", () => { const obj = {a: {b: {}}} const next = produce({}, () => obj) @@ -90,37 +82,43 @@ function runTests(name, useProxies) { expect(isFrozen(next.a.b)).toBeTruthy() expect(isFrozen(next.a.b.c)).toBeTruthy() }) - }) - describe("the result is never auto-frozen when", () => { + it("a nested draft is returned", () => { + const base = {a: {}} + const next = produce(base, draft => draft.a) + expect(next).toBe(base.a) + expect(isFrozen(next)).toBeTruthy() + }) + + it("the base state is returned", () => { + const base = {} + const next = produce(base, () => base) + expect(next).toBe(base) + expect(isFrozen(next)).toBeTruthy() + }) + it("the producer is a no-op", () => { const base = {a: {}} const next = produce(base, () => {}) expect(next).toBe(base) - expect(isFrozen(next)).toBeFalsy() - expect(isFrozen(next.a)).toBeFalsy() + expect(isFrozen(next)).toBeTruthy() + expect(isFrozen(next.a)).toBeTruthy() }) it("the root draft is returned", () => { const base = {a: {}} const next = produce(base, draft => draft) expect(next).toBe(base) - expect(isFrozen(next)).toBeFalsy() - expect(isFrozen(next.a)).toBeFalsy() - }) - - it("a nested draft is returned", () => { - const base = {a: {}} - const next = produce(base, draft => draft.a) - expect(next).toBe(base.a) - expect(isFrozen(next)).toBeFalsy() + expect(isFrozen(next)).toBeTruthy() + expect(isFrozen(next.a)).toBeTruthy() }) - it("the base state is returned", () => { - const base = {} - const next = produce(base, () => base) - expect(next).toBe(base) - expect(isFrozen(next)).toBeFalsy() + it("a new object replaces a primitive base", () => { + const obj = {a: {}} + const next = produce(null, () => obj) + expect(next).toBe(obj) + expect(isFrozen(next)).toBeTruthy() + expect(isFrozen(next.a)).toBeTruthy() }) }) diff --git a/readme.md b/readme.md index 92a44997..3e2b32a5 100644 --- a/readme.md +++ b/readme.md @@ -548,6 +548,8 @@ isDraft(nextState) // => false Immer automatically freezes any state trees that are modified using `produce`. This protects against accidental modifications of the state tree outside of a producer. This comes with a performance impact, so it is recommended to disable this option in production. It is by default enabled. By default, it is turned on during local development and turned off in production. Use `setAutoFreeze(true / false)` to explicitly turn this feature on or off. +_⚠️ If auto freezing is enabled, recipes are not entirely side-effect free: Any plain object or array that ends up in the produced result, will be frozen, even when these objects were not frozen before the start of the producer! ⚠️_ + ## Immer on older JavaScript environments? By default `produce` tries to use proxies for optimal performance. However, on older JavaScript engines `Proxy` is not available. For example, when running Microsoft Internet Explorer or React Native (< v0.59) on Android. In such cases, Immer will fallback to an ES5 compatible implementation which works identical, but is a bit slower. diff --git a/src/common.js b/src/common.js index 6abb39ed..2e2dfd93 100644 --- a/src/common.js +++ b/src/common.js @@ -120,3 +120,12 @@ export function clone(obj) { for (const key in obj) cloned[key] = clone(obj[key]) return cloned } + +export function deepFreeze(obj) { + if (!isDraftable(obj)) return + if (Object.isFrozen(obj)) return + if (isDraft(obj)) return + Object.freeze(obj) + if (Array.isArray(obj)) obj.forEach(deepFreeze) + for (const key in obj) deepFreeze(obj[key]) +} diff --git a/src/immer.js b/src/immer.js index 2159bbca..489a61bb 100644 --- a/src/immer.js +++ b/src/immer.js @@ -11,7 +11,8 @@ import { isEnumerable, shallowCopy, DRAFT_STATE, - NOTHING + NOTHING, + deepFreeze } from "./common" import {ImmerScope} from "./scope" @@ -87,8 +88,10 @@ export class Immer { return this.processResult(result, scope) } else { result = recipe(base) - if (result === undefined) return base - return result !== NOTHING ? result : undefined + if (result === NOTHING) return undefined + if (result === undefined) result = base + this.maybeFreeze(result, true) + return result } } produceWithPatches(arg1, arg2, arg3) { @@ -170,6 +173,7 @@ export class Immer { if (isDraftable(result)) { // Finalize the result in case it contains (or is) a subset of the draft. result = this.finalize(result, null, scope) + this.maybeFreeze(result) } if (scope.patches) { scope.patches.push({ @@ -209,6 +213,7 @@ export class Immer { return draft } if (!state.modified) { + this.maybeFreeze(state.base, true) return state.base } if (!state.finalized) { @@ -299,6 +304,8 @@ export class Immer { // Search new objects for unfinalized drafts. Frozen objects should never contain drafts. else if (isDraftable(value) && !Object.isFrozen(value)) { each(value, finalizeProperty) + // for externally incoming object trees, we wan't to make sure they're frozen automatically + this.maybeFreeze(value) } if (isDraftProp && this.onAssign) { @@ -309,4 +316,10 @@ export class Immer { each(root, finalizeProperty) return root } + maybeFreeze(value, deep = false) { + if (this.autoFreeze && !isDraft(value)) { + if (deep) deepFreeze(value) + else Object.freeze(value) + } + } } From 9064d26aaaa4e6d5cc447b1b140f4c891286e813 Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Wed, 11 Sep 2019 22:31:19 +0200 Subject: [PATCH 3/5] Processed review comments --- src/common.js | 6 ++---- src/immer.js | 1 - 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/src/common.js b/src/common.js index 2e2dfd93..65f5e150 100644 --- a/src/common.js +++ b/src/common.js @@ -122,10 +122,8 @@ export function clone(obj) { } export function deepFreeze(obj) { - if (!isDraftable(obj)) return - if (Object.isFrozen(obj)) return - if (isDraft(obj)) return + if (!isDraftable(obj) || isDraft(obj) || Object.isFrozen(obj)) return Object.freeze(obj) if (Array.isArray(obj)) obj.forEach(deepFreeze) - for (const key in obj) deepFreeze(obj[key]) + else for (const key in obj) deepFreeze(obj[key]) } diff --git a/src/immer.js b/src/immer.js index 489a61bb..2727f7c8 100644 --- a/src/immer.js +++ b/src/immer.js @@ -304,7 +304,6 @@ export class Immer { // Search new objects for unfinalized drafts. Frozen objects should never contain drafts. else if (isDraftable(value) && !Object.isFrozen(value)) { each(value, finalizeProperty) - // for externally incoming object trees, we wan't to make sure they're frozen automatically this.maybeFreeze(value) } From fc41507b84a52764fe62501596b3c851e0c586a9 Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Wed, 11 Sep 2019 22:50:56 +0200 Subject: [PATCH 4/5] BREAKING CHANGE: In development mode, any new value stored in a tree will be deeply frozen This introduces a new side effect, where any data returned for a producer will be frozen. See #417 and linked issues for details --- readme.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/readme.md b/readme.md index 5966e0b4..dd7a37e4 100644 --- a/readme.md +++ b/readme.md @@ -833,3 +833,5 @@ Special thanks to @Mendix, which supports its employees to experiment completely ## Donations A significant part of my OSS work is unpaid. So [donations](https://mobx.js.org/donate.html) are greatly appreciated :) + +. From cb1c6dd8a33073aaa0a4d881c94ec7ab1c1be7f6 Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Wed, 11 Sep 2019 23:11:00 +0200 Subject: [PATCH 5/5] feat: In development mode, any new value stored in a tree will be deeply frozen. BREAKING CHANGE: any value returned from produce will be deeply frozen in DEV, even when it wasn't frozen before the producer started. Fixes #412 #363 #260 #230 #313 #229 through #417 --- readme.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/readme.md b/readme.md index dd7a37e4..5966e0b4 100644 --- a/readme.md +++ b/readme.md @@ -833,5 +833,3 @@ Special thanks to @Mendix, which supports its employees to experiment completely ## Donations A significant part of my OSS work is unpaid. So [donations](https://mobx.js.org/donate.html) are greatly appreciated :) - -.