From 7836fb060fb23b8938e147bfd0dab14d9da0bd4e Mon Sep 17 00:00:00 2001 From: Grzegorz Rozdzialik Date: Thu, 3 Dec 2020 22:27:26 +0100 Subject: [PATCH 1/5] feature: Add strict mode for accessing non-draftables In strict mode, accessing a non-draftable property will throw One can use the unsafe function to perform such operations Fixes #686 --- src/core/immerClass.ts | 27 ++++++++++++++++++++++++++- src/core/proxy.ts | 14 +++++++++++++- src/core/scope.ts | 4 +++- src/immer.ts | 12 ++++++++++++ src/types/index.js.flow | 12 ++++++++++++ src/utils/errors.ts | 3 ++- 6 files changed, 68 insertions(+), 4 deletions(-) diff --git a/src/core/immerClass.ts b/src/core/immerClass.ts index f98d3756..c80c7ba5 100644 --- a/src/core/immerClass.ts +++ b/src/core/immerClass.ts @@ -37,11 +37,19 @@ export class Immer implements ProducersFns { autoFreeze_: boolean = true - constructor(config?: {useProxies?: boolean; autoFreeze?: boolean}) { + strictModeEnabled_: boolean = false + + constructor(config?: { + useProxies?: boolean + autoFreeze?: boolean + strictMode?: boolean + }) { if (typeof config?.useProxies === "boolean") this.setUseProxies(config!.useProxies) if (typeof config?.autoFreeze === "boolean") this.setAutoFreeze(config!.autoFreeze) + if (typeof config?.strictMode === "boolean") + this.setStrictMode(config!.strictMode) this.produce = this.produce.bind(this) this.produceWithPatches = this.produceWithPatches.bind(this) } @@ -183,6 +191,23 @@ export class Immer implements ProducersFns { this.useProxies_ = value } + /** + * Pass true to throw errors when attempting to access a non-draftable reference. + * + * By default, strict mode is disabled. + */ + setStrictMode(value: boolean) { + this.strictModeEnabled_ = value + } + + unsafe(callback: () => void) { + const scope = getCurrentScope() + + scope.unsafeNonDraftabledAllowed_ = true + callback() + scope.unsafeNonDraftabledAllowed_ = false + } + applyPatches(base: Objectish, patches: Patch[]) { // If a patch replaces the entire state, take that replacement as base // before applying patches diff --git a/src/core/proxy.ts b/src/core/proxy.ts index 2469d73e..319af4fb 100644 --- a/src/core/proxy.ts +++ b/src/core/proxy.ts @@ -108,7 +108,19 @@ export const objectTraps: ProxyHandler = { return readPropFromProto(state, source, prop) } const value = source[prop] - if (state.finalized_ || !isDraftable(value)) { + if (state.finalized_) { + return value + } + if (!isDraftable(value)) { + if ( + state.scope_.immer_.strictModeEnabled_ && + !state.scope_.unsafeNonDraftabledAllowed_ && + typeof value === "object" && + value !== null + ) { + die(24) + } + return value } // Check for existing draft in modified state. diff --git a/src/core/scope.ts b/src/core/scope.ts index 4505ea63..90bbe893 100644 --- a/src/core/scope.ts +++ b/src/core/scope.ts @@ -22,6 +22,7 @@ export interface ImmerScope { patchListener_?: PatchListener immer_: Immer unfinalizedDrafts_: number + unsafeNonDraftabledAllowed_: boolean } let currentScope: ImmerScope | undefined @@ -42,7 +43,8 @@ function createScope( // Whenever the modified draft contains a draft from another scope, we // need to prevent auto-freezing so the unowned draft can be finalized. canAutoFreeze_: true, - unfinalizedDrafts_: 0 + unfinalizedDrafts_: 0, + unsafeNonDraftabledAllowed_: false } } diff --git a/src/immer.ts b/src/immer.ts index 53455494..e3e6e1ab 100644 --- a/src/immer.ts +++ b/src/immer.ts @@ -67,6 +67,18 @@ export const setAutoFreeze = immer.setAutoFreeze.bind(immer) */ export const setUseProxies = immer.setUseProxies.bind(immer) +/** + * Pass true to throw errors when attempting to access a non-draftable reference. + * + * By default, strict mode is disabled. + */ +export const setStrictMode = immer.setStrictMode.bind(immer) + +/** + * Allow accessing non-draftable references in strict mode inside the callback. + */ +export const unsafe = immer.unsafe.bind(immer) + /** * Apply an array of Immer patches to the first argument. * diff --git a/src/types/index.js.flow b/src/types/index.js.flow index 1be017a2..c6e742f7 100644 --- a/src/types/index.js.flow +++ b/src/types/index.js.flow @@ -84,6 +84,18 @@ declare export function setAutoFreeze(autoFreeze: boolean): void */ declare export function setUseProxies(useProxies: boolean): void +/** + * Pass true to throw errors when attempting to access a non-draftable reference. + * + * By default, strict mode is disabled. + */ +declare export function setStrictMode(strictMode: boolean): void + +/** + * Allow accessing non-draftable references in strict mode inside the callback. + */ +declare export function unsafe(callback: () => void): void + declare export function applyPatches(state: S, patches: Patch[]): S declare export function original(value: S): S diff --git a/src/utils/errors.ts b/src/utils/errors.ts index b73e2aac..7d158bb0 100644 --- a/src/utils/errors.ts +++ b/src/utils/errors.ts @@ -38,7 +38,8 @@ const errors = { }, 23(thing: string) { return `'original' expects a draft, got: ${thing}` - } + }, + 24: "Cannot get a non-draftable reference in strict mode. Use the `unsafe` function, add the `immerable` symbol, or disable strict mode" } as const export function die(error: keyof typeof errors, ...args: any[]): never { From c30fef3bfefcf1feddb01befad067583e7849b0a Mon Sep 17 00:00:00 2001 From: Grzegorz Rozdzialik Date: Thu, 3 Dec 2020 23:09:10 +0100 Subject: [PATCH 2/5] Add tests for strict mode --- __tests__/strict-mode.js | 106 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 106 insertions(+) create mode 100644 __tests__/strict-mode.js diff --git a/__tests__/strict-mode.js b/__tests__/strict-mode.js new file mode 100644 index 00000000..0681715b --- /dev/null +++ b/__tests__/strict-mode.js @@ -0,0 +1,106 @@ +"use strict" +import produce, { + setStrictMode, + unsafe, + immerable, + enableMapSet +} from "../src/immer" + +enableMapSet() + +describe("Strict Mode", () => { + class Foo {} + + describe("by default", () => { + it("should not throw an error when accessing a non-draftable class instance", () => { + expect(() => + produce({instance: new Foo()}, draft => { + draft.instance.value = 5 + }) + ).not.toThrow() + }) + }) + + afterAll(() => { + setStrictMode(false) + }) + + describe("when disabled", () => { + beforeEach(() => { + setStrictMode(false) + }) + + it("should allow accessing a non-draftable class instance", () => { + expect(() => + produce({instance: new Foo()}, draft => { + draft.instance.value = 5 + }) + ).not.toThrow() + }) + + it("should not throw errors when using the `unsafe` function", () => { + expect(() => + produce({instance: new Foo()}, draft => { + unsafe(() => { + draft.instance.value = 5 + }) + }) + ).not.toThrow() + }) + }) + + describe("when enabled", () => { + beforeEach(() => { + setStrictMode(true) + }) + + it("should throw an error when accessing a non-draftable class instance", () => { + expect(() => + produce({instance: new Foo()}, draft => { + draft.instance + }) + ).toThrow() + }) + + it("should allow accessing a non-draftable using the `unsafe` function", () => { + expect(() => + produce({instance: new Foo()}, draft => { + unsafe(() => { + draft.instance.value = 5 + }) + }) + ).not.toThrow() + }) + + describe("with an immerable class", () => { + beforeAll(() => { + Foo[immerable] = true + }) + + afterAll(() => { + Foo[immerable] = true + }) + + it("should allow accessing the class instance", () => { + expect(() => + produce({instance: new Foo()}, draft => { + draft.instance.value = 5 + }) + ).not.toThrow() + }) + }) + + it("should allow accessing draftable properties", () => { + expect(() => + produce({arr: [], obj: {}, map: new Map(), set: new Set()}, draft => { + draft.arr.push(1) + draft.arr[0] = 1 + draft.obj.foo = 5 + draft.obj.hasOwnProperty("abc") + draft.map.set("foo", 5) + draft.set.add("foo") + }) + ).not.toThrow() + }) + }) +}) From ae6808e29c434a5ae7d8a1581189b48248a6ec84 Mon Sep 17 00:00:00 2001 From: Grzegorz Rozdzialik Date: Thu, 3 Dec 2020 23:39:58 +0100 Subject: [PATCH 3/5] Allow using strict mode with ES5 plugin --- src/plugins/es5.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/plugins/es5.ts b/src/plugins/es5.ts index d0c3e02d..45f6ec7f 100644 --- a/src/plugins/es5.ts +++ b/src/plugins/es5.ts @@ -31,7 +31,9 @@ export function enableES5() { ) { if (!isReplaced) { if (scope.patches_) { + scope.unsafeNonDraftabledAllowed_ = true markChangesRecursively(scope.drafts_![0]) + scope.unsafeNonDraftabledAllowed_ = false } // This is faster when we don't care about which attributes changed. markChangesSweep(scope.drafts_) From adeb53b0997fb4b6cac2765911e3656b0ef6f8ab Mon Sep 17 00:00:00 2001 From: Grzegorz Rozdzialik Date: Fri, 4 Dec 2020 07:51:19 +0100 Subject: [PATCH 4/5] Add test for non-draftables in a different scope --- __tests__/strict-mode.js | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/__tests__/strict-mode.js b/__tests__/strict-mode.js index 0681715b..c4420fcb 100644 --- a/__tests__/strict-mode.js +++ b/__tests__/strict-mode.js @@ -72,6 +72,22 @@ describe("Strict Mode", () => { ).not.toThrow() }) + it("should require using unsafe for non-draftables in a different scope", () => { + expect.assertions(2) + + produce({instance: new Foo()}, () => { + unsafe(() => { + produce({nested: new Foo()}, nestedDraft => { + expect(() => nestedDraft.nested).toThrow() + + unsafe(() => { + expect(() => nestedDraft.nested).not.toThrow() + }) + }) + }) + }) + }) + describe("with an immerable class", () => { beforeAll(() => { Foo[immerable] = true From c60c592bd26eb1fdca2644e5a0571e711762fcd1 Mon Sep 17 00:00:00 2001 From: Grzegorz Rozdzialik Date: Fri, 4 Dec 2020 07:55:41 +0100 Subject: [PATCH 5/5] Reduce the scope of expects in strict-mode tests --- __tests__/strict-mode.js | 64 +++++++++++++++++++++++----------------- 1 file changed, 37 insertions(+), 27 deletions(-) diff --git a/__tests__/strict-mode.js b/__tests__/strict-mode.js index c4420fcb..7328a35e 100644 --- a/__tests__/strict-mode.js +++ b/__tests__/strict-mode.js @@ -13,11 +13,13 @@ describe("Strict Mode", () => { describe("by default", () => { it("should not throw an error when accessing a non-draftable class instance", () => { - expect(() => - produce({instance: new Foo()}, draft => { + expect.hasAssertions() + + produce({instance: new Foo()}, draft => { + expect(() => { draft.instance.value = 5 - }) - ).not.toThrow() + }).not.toThrow() + }) }) }) @@ -31,21 +33,25 @@ describe("Strict Mode", () => { }) it("should allow accessing a non-draftable class instance", () => { - expect(() => - produce({instance: new Foo()}, draft => { + expect.hasAssertions() + + produce({instance: new Foo()}, draft => { + expect(() => { draft.instance.value = 5 - }) - ).not.toThrow() + }).not.toThrow() + }) }) it("should not throw errors when using the `unsafe` function", () => { - expect(() => - produce({instance: new Foo()}, draft => { - unsafe(() => { + expect.hasAssertions() + + produce({instance: new Foo()}, draft => { + unsafe(() => { + expect(() => { draft.instance.value = 5 - }) + }).not.toThrow() }) - ).not.toThrow() + }) }) }) @@ -55,21 +61,23 @@ describe("Strict Mode", () => { }) it("should throw an error when accessing a non-draftable class instance", () => { - expect(() => - produce({instance: new Foo()}, draft => { - draft.instance - }) - ).toThrow() + expect.hasAssertions() + + produce({instance: new Foo()}, draft => { + expect(() => draft.instance).toThrow() + }) }) it("should allow accessing a non-draftable using the `unsafe` function", () => { - expect(() => - produce({instance: new Foo()}, draft => { - unsafe(() => { + expect.hasAssertions() + + produce({instance: new Foo()}, draft => { + unsafe(() => { + expect(() => { draft.instance.value = 5 - }) + }).not.toThrow() }) - ).not.toThrow() + }) }) it("should require using unsafe for non-draftables in a different scope", () => { @@ -98,11 +106,13 @@ describe("Strict Mode", () => { }) it("should allow accessing the class instance", () => { - expect(() => - produce({instance: new Foo()}, draft => { + expect.hasAssertions() + + produce({instance: new Foo()}, draft => { + expect(() => { draft.instance.value = 5 - }) - ).not.toThrow() + }).not.toThrow() + }) }) })