Added support for frozen trees, and automatically freeze generated trees #15
Conversation
| * 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) |
There was a problem hiding this comment.
The return type should be void instead of implicit any
export function setAutoFreeze(autoFreeze: boolean):voidThis is to prevent the use of setAutoFreeze return value in an unchecked way. Here is an example:
const a = setAutoFreeze(false)
a.test(); // error if the return type of setAutoFreeze is void, but will be ok if return type is any
immer.js
Outdated
| 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 } |
There was a problem hiding this comment.
Since spread operator is used, babel-plugin-transform-object-rest-spread should be added as a dev dependecny. Also, .babelrc should be updated with "plugins": ["transform-object-rest-spread"]
There was a problem hiding this comment.
Good catch, actually I did not put the config there on purpose, to make sure the js files works on modern environments without compilation. I am a bit suprised Jest is fine with this (maybe it is because I switched to node 8). Probably I should add transpilation step soonish...
| draft.object.c = 2 | ||
| }) | ||
| expect(Object.isFrozen(next.object)).toBe(true) | ||
| expect(Object.isFrozen(next)).toBe(true) |
There was a problem hiding this comment.
Should we also safely assert that next.array is not forzen in the process ?
__tests__/frozen.js
Outdated
| const n = immer(b, draft => { | ||
| draft.c = true | ||
| draft.a.push(3) | ||
| debugger |
There was a problem hiding this comment.
Should debugger statement be removed ?
|
@benbraou thanks for the review! Interested in becoming a maintainer for this package? |
|
@mweststrate thank you. Yes, I am interested in becoming a maintainer! |
Implemens #2