From ab8987f7f5b50951a0366b3ef153f8f3f9b6ad53 Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Sun, 31 Dec 2017 19:48:49 +0100 Subject: [PATCH 1/5] Added support for frozen trees, and automatically freeze generated trees --- __tests__/base.js | 38 ++---------------------- __tests__/frozen.js | 49 +++++++++++++++++++++++++++++++ __tests__/readme.js | 36 +++++++++++++++++++++++ immer.js | 71 ++++++++++++++++++++++++++++++++++++--------- index.d.ts | 12 ++++++-- readme.md | 15 ++++++++++ 6 files changed, 170 insertions(+), 51 deletions(-) create mode 100644 __tests__/frozen.js create mode 100644 __tests__/readme.js diff --git a/__tests__/base.js b/__tests__/base.js index 34d4fc65..36816315 100644 --- a/__tests__/base.js +++ b/__tests__/base.js @@ -1,3 +1,4 @@ +"use strict" import immer from ".." describe("base", () => { @@ -127,7 +128,6 @@ describe("base", () => { it("reusing object should work", () => { const nextState = immer(baseState, s => { - debugger const obj = s.anObject delete s.anObject s.messy = obj @@ -175,7 +175,7 @@ describe("base", () => { s.aProp = undefined }) expect(nextState).not.toBe(baseState) - expect(baseState.aProp).toBe('hi') + expect(baseState.aProp).toBe("hi") expect(nextState.aProp).toBe(undefined) }) @@ -197,37 +197,3 @@ describe("base", () => { } } }) - -describe("readme example", () => { - it("works", () => { - const baseState = [ - { - todo: "Learn typescript", - done: true - }, - { - todo: "Try immer", - done: false - } - ] - - const nextState = immer(baseState, state => { - state.push({ todo: "Tweet about it" }) - state[1].done = true - }) - - // the new item is only added to the next state, - // base state is unmodified - expect(baseState.length).toBe(2) - expect(nextState.length).toBe(3) - - // same for the changed 'done' prop - expect(baseState[1].done).toBe(false) - expect(nextState[1].done).toBe(true) - - // unchanged data is structurally shared - expect(nextState[0]).toBe(baseState[0]) - // changed data not (dûh) - expect(nextState[1]).not.toBe(baseState[1]) - }) -}) diff --git a/__tests__/frozen.js b/__tests__/frozen.js new file mode 100644 index 00000000..e1d4874a --- /dev/null +++ b/__tests__/frozen.js @@ -0,0 +1,49 @@ +"use strict" +import immer from ".." + +describe("auto freeze", () => { + const baseState = { + object: { a: 1 }, + array: [1, 2] + } + + it("should freeze objects after modifications", () => { + expect(Object.isFrozen(baseState.object)).toBe(false) // initially not frozen + const next = immer(baseState, draft => { + draft.object.c = 2 + }) + expect(Object.isFrozen(next.object)).toBe(true) + expect(Object.isFrozen(next)).toBe(true) + + expect(() => { + next.object.a = 2 + }).toThrow(/Cannot assign to read only property/) + }) + + it("should freeze arrays after modifications", () => { + expect(Object.isFrozen(baseState.object)).toBe(false) // initially not frozen + const next = immer(baseState, draft => { + draft.array.push(3) + }) + expect(Object.isFrozen(next.object)).toBe(false) // not touched + expect(Object.isFrozen(next)).toBe(true) + expect(Object.isFrozen(next.array)).toBe(true) + + expect(() => { + next.array.shift() + }).toThrow(/Cannot add\/remove sealed array elements/) + }) + + it("can handle already frozen trees", () => { + const a = [] + const b = { a: a } + Object.freeze(a) + Object.freeze(b) + const n = immer(b, draft => { + draft.c = true + draft.a.push(3) + debugger + }) + expect(n).toEqual({ c: true, a: [3] }) + }) +}) diff --git a/__tests__/readme.js b/__tests__/readme.js new file mode 100644 index 00000000..0dbb35a3 --- /dev/null +++ b/__tests__/readme.js @@ -0,0 +1,36 @@ +"use strict" +import immer from ".." + +describe("readme example", () => { + it("works", () => { + const baseState = [ + { + todo: "Learn typescript", + done: true + }, + { + todo: "Try immer", + done: false + } + ] + + const nextState = immer(baseState, draft => { + draft.push({ todo: "Tweet about it" }) + draft[1].done = true + }) + + // the new item is only added to the next state, + // base state is unmodified + expect(baseState.length).toBe(2) + expect(nextState.length).toBe(3) + + // same for the changed 'done' prop + expect(baseState[1].done).toBe(false) + expect(nextState[1].done).toBe(true) + + // unchanged data is structurally shared + expect(nextState[0]).toBe(baseState[0]) + // changed data not (dûh) + expect(nextState[1]).not.toBe(baseState[1]) + }) +}) diff --git a/immer.js b/immer.js index bea9e782..6a1ba91d 100644 --- a/immer.js +++ b/immer.js @@ -1,6 +1,13 @@ +"use strict" // @ts-check -const isProxySymbol = Symbol("immer-proxy") +const IMMER_PROXY = Symbol("immer-proxy") // TODO: create per closure, to avoid sharing proxies between multiple immer version + +// This property indicates that the current object is cloned for another object, +// to make sure the proxy of a frozen object is writeable +const CLONE_TARGET = Symbol("immer-clone-target") + +let autoFreeze = true /** * Immer takes a state, and runs a function against it. @@ -20,7 +27,7 @@ function immer(baseState, thunk) { const objectTraps = { get(target, prop) { - if (prop === isProxySymbol) return target + if (prop === IMMER_PROXY) return target return createProxy(getCurrentSource(target)[prop]) }, has(target, prop) { @@ -34,7 +41,7 @@ function immer(baseState, thunk) { const newValue = createProxy(value) if (current !== newValue) { const copy = getOrCreateCopy(target) - copy[prop] = isProxy(newValue) ? newValue[isProxySymbol] : newValue + copy[prop] = isProxy(newValue) ? newValue[IMMER_PROXY] : newValue } return true }, @@ -48,10 +55,16 @@ function immer(baseState, thunk) { // creates a copy for a base object if there ain't one function getOrCreateCopy(base) { let copy = copies.get(base) - if (!copy) { - copy = Array.isArray(base) ? base.slice() : Object.assign({}, base) - copies.set(base, copy) + if (copy) return copy + const cloneTarget = base[CLONE_TARGET] + if (cloneTarget) { + // base is a clone already (source was frozen), no need to create addtional copy + copies.set(cloneTarget, base) + return base } + // create a fresh copy + copy = Array.isArray(base) ? base.slice() : Object.assign({}, base) + copies.set(base, copy) return copy } @@ -66,7 +79,18 @@ function immer(baseState, thunk) { if (isPlainObject(base) || Array.isArray(base)) { if (isProxy(base)) return base // avoid double wrapping if (proxies.has(base)) return proxies.get(base) - const proxy = new Proxy(base, objectTraps) + let proxyTarget = base + // special case, if the base tree is frozen, we cannot modify it's proxy it in strict mode, clone first. + if (Object.isFrozen(proxyTarget)) { + proxyTarget = Array.isArray(proxyTarget) ? proxyTarget.slice() : { ...proxyTarget } + Object.defineProperty(proxyTarget, CLONE_TARGET, { + enumerable: false, + value: base, + configurable: true + }) + } + // create the proxy + const proxy = new Proxy(proxyTarget, objectTraps) proxies.set(base, proxy) return proxy } @@ -97,20 +121,22 @@ function immer(baseState, thunk) { function finalizeObject(thing) { if (!hasChanges(thing)) return thing - const copy = getOrCreateCopy(thing) + const copy = getOrCreateCopy(thing) // TODO: getOrCreate is weird here.. Object.keys(copy).forEach(prop => { copy[prop] = finalize(copy[prop]) }) - return copy + delete copy[CLONE_TARGET] + return freeze(copy) } function finalizeArray(thing) { if (!hasChanges(thing)) return thing - const copy = getOrCreateCopy(thing) + const copy = getOrCreateCopy(thing) // TODO: getOrCreate is weird here.. copy.forEach((value, index) => { copy[index] = finalize(copy[index]) }) - return copy + delete copy[CLONE_TARGET] + return freeze(copy) } // create proxy for root @@ -128,7 +154,26 @@ function isPlainObject(value) { } function isProxy(value) { - return !!value && !!value[isProxySymbol] + return !!value && !!value[IMMER_PROXY] +} + +function freeze(value) { + if (autoFreeze) { + Object.freeze(value) + } + return value +} + +/** + * Automatically freezes any state trees generated by immer. + * This protects against accidental modifications of the state tree outside of an immer function. + * This comes with a performance impact, so it is recommended to disable this option in production. + * It is by default enabled. + */ +function setAutoFreeze(enableAutoFreeze) { + autoFreeze = enableAutoFreeze } -module.exports = immer +module.exports.__esModule = true +module.exports.default = immer +module.exports.setAutoFreeze = setAutoFreeze diff --git a/index.d.ts b/index.d.ts index e88cad8e..d87dd1bb 100644 --- a/index.d.ts +++ b/index.d.ts @@ -2,9 +2,17 @@ * Immer takes a state, and runs a function against it. * That function can freely mutate the state, as it will create copies-on-write. * This means that the original state will stay unchanged, and once the function finishes, the modified state is returned - * + * * @param currentState - the state to start with * @param thunk - function that receives a proxy of the current state as first argument and which can be freely modified * @returns The next state: a new state, or the current state if nothing was modified */ -export function immer(currentState: S, thunk: (draftState: S) => void): S; +export function immer(currentState: S, thunk: (draftState: S) => void): S + +/** + * Automatically freezes any state trees generated by immer. + * This protects against accidental modifications of the state tree outside of an immer function. + * This comes with a performance impact, so it is recommended to disable this option in production. + * It is by default enabled. + */ +export function setAutoFreeze(autoFreeze: boolean) diff --git a/readme.md b/readme.md index a513baf5..03fc3930 100644 --- a/readme.md +++ b/readme.md @@ -68,6 +68,15 @@ expect(nextState[1]).not.toBe(baseState[1]) * Small, dependency free library with minimal api surface * No accidental mutations of current state, but intentional mutations of a draft state +## Auto freezing + + Immer automatically freezes any state trees that are modified using `immer. + This protects against accidental modifications of the state tree outside of an immer function. + This comes with a performance impact, so it is recommended to disable this option in production. + It is by default enabled. + + Use `setAutoFreeze(true / false)` to turn this feature on or off. + ## Reducer Example A lot of words; here is a simple example of what difference that could make in practice. @@ -154,6 +163,12 @@ Creating middleware or reducer wrapper that applies `immer` automatically is lef ## Changelog +### 0.0.5 + +* Fixed `immer` function export, it is now properly exposed as `default` +* Immer now automatically freezes any state modifications made. Turn this is using `setAutoFreeze(false)` +* Added support for frozen state trees in strict mode. + ### 0.0.4 (31-12-2017) * Added typescript typings [#11](https://github.com/mweststrate/immer/pull/11) by [@benbraou](https://github.com/benbraou) From 0190958af9ce6b84544170945230608e91fd8585 Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Sun, 31 Dec 2017 20:14:29 +0100 Subject: [PATCH 2/5] `__esModule` should be non-enumerable --- immer.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/immer.js b/immer.js index 6a1ba91d..4073b1f4 100644 --- a/immer.js +++ b/immer.js @@ -174,6 +174,8 @@ function setAutoFreeze(enableAutoFreeze) { autoFreeze = enableAutoFreeze } -module.exports.__esModule = true +Object.defineProperty(exports, "__esModule", { + value: true +}) module.exports.default = immer module.exports.setAutoFreeze = setAutoFreeze From 53a6f875c3e6bf158c0c0cda368317c7ecebda3e Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Mon, 1 Jan 2018 19:58:54 +0100 Subject: [PATCH 3/5] Fixed merge issue --- immer.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/immer.js b/immer.js index 04bc4ca4..1de5bf50 100644 --- a/immer.js +++ b/immer.js @@ -105,7 +105,7 @@ function immer(baseState, thunk) { }) } // create the proxy - const revocableProxy = Proxy.revocable(base, objectTraps) + const revocableProxy = Proxy.revocable(proxyTarget, objectTraps) revocableProxies.set(base, revocableProxy) return revocableProxy.proxy } From 69d93b707f4efdf93e8d8e679bfc513f80d605e0 Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Mon, 1 Jan 2018 20:03:18 +0100 Subject: [PATCH 4/5] Processed review comments --- __tests__/frozen.js | 2 +- immer.js | 14 +++++++++----- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/__tests__/frozen.js b/__tests__/frozen.js index f6ef5f1d..63e0cb2f 100644 --- a/__tests__/frozen.js +++ b/__tests__/frozen.js @@ -14,6 +14,7 @@ describe("auto freeze", () => { }) expect(Object.isFrozen(next.object)).toBe(true) expect(Object.isFrozen(next)).toBe(true) + expect(Object.isFrozen(next.array)).toBe(false) expect(() => { next.object.a = 2 @@ -42,7 +43,6 @@ describe("auto freeze", () => { const n = immer(b, draft => { draft.c = true draft.a.push(3) - debugger }) expect(n).toEqual({c: true, a: [3]}) }) diff --git a/immer.js b/immer.js index 1de5bf50..f54bc85a 100644 --- a/immer.js +++ b/immer.js @@ -92,17 +92,19 @@ function immer(baseState, thunk) { if (isProxy(base)) return base // avoid double wrapping if (revocableProxies.has(base)) return revocableProxies.get(base).proxy - let proxyTarget = base + let proxyTarget // special case, if the base tree is frozen, we cannot modify it's proxy it in strict mode, clone first. - if (Object.isFrozen(proxyTarget)) { - proxyTarget = Array.isArray(proxyTarget) - ? proxyTarget.slice() - : {...proxyTarget} + if (Object.isFrozen(base)) { + proxyTarget = Array.isArray(base) + ? base.slice() + : Object.assign({}, base) Object.defineProperty(proxyTarget, CLONE_TARGET, { enumerable: false, value: base, configurable: true, }) + } else { + proxyTarget = base } // create the proxy const revocableProxy = Proxy.revocable(proxyTarget, objectTraps) @@ -206,6 +208,8 @@ function freeze(value) { * This protects against accidental modifications of the state tree outside of an immer function. * This comes with a performance impact, so it is recommended to disable this option in production. * It is by default enabled. + * + * @returns {void} */ function setAutoFreeze(enableAutoFreeze) { autoFreeze = enableAutoFreeze From 84905e7630194337714517e7640a227d78b3b216 Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Mon, 1 Jan 2018 20:06:27 +0100 Subject: [PATCH 5/5] `immer` should be the default export --- index.d.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/index.d.ts b/index.d.ts index d87dd1bb..2f4a2e7d 100644 --- a/index.d.ts +++ b/index.d.ts @@ -7,7 +7,10 @@ * @param thunk - function that receives a proxy of the current state as first argument and which can be freely modified * @returns The next state: a new state, or the current state if nothing was modified */ -export function immer(currentState: S, thunk: (draftState: S) => void): S +export default function( + currentState: S, + thunk: (draftState: S) => void, +): S /** * Automatically freezes any state trees generated by immer.