Immer does not work with a polyfilled Symbol implementation

[timothympace]

🐛 Bug Report

This bug only occurs when using the ES5 implementation (no proxies) and there is a polyfilled version of Symbol on the window object (in my case this is reproducible both with core-js and es6-symbol). I already investigated and found where the root cause is, though I am not sure what the best route forward is (I have several suggestions -- see below)

Link to repro

Unfortunately, this is VERY hard to reproduce in codesandbox. I tried importing polyfills in codesandbox and overwriting the native implementation, but it was VERY finicky and I couldn't get it to work. Hopefully the root cause explained below is enough to go on 😄

Root Cause

The issue occurs because there are checks within the immer codebase like this:

if (key === DRAFT_STATE) {
  // do something
}

One for instance, is in shallowCopy. In that case (and other similar ones), the draftState object is passed into each where it will iterate through the keys (including the Symbol key used to store draft state). Since Symbol is polyfilled, the draft state key will be stringified and no longer pass the check for key === DRAFT_STATE, since the Symbol polyfill is not native and cannot be attached to an object without being stringified. The native Symbol produces objects with a special typeof === 'symbol whereas the polyfill produces a plain object that can't follow the rules of Symbol. In the case of shallowCopy, things blow up when the draft state key is not skipped (like the comment says it should)

Potential Ideas

1. Change checks like this to be key == DRAFT_STATE
   • Loose equals will work for when key is a string and DRAFT_STATE is an object because DRAFT_STATE stringifies to the key value.
   • Loose equals also works with the native implementation.
2. Change the check to be typeof DRAFT_STATE === 'symbol' ? key === DRAFT_STATE : key === DRAFTE_STATE.toString()
   • Safely falls back to a string check if the DRAFT_STATE is not a real symbol.
3. Change hasSymbol to be const hasSymbol = typeof Symbol !== "undefined" && typeof Symbol("test") === 'symbol'
   • Esentially, only native Symbol definitions would be eligible.
   • This way things like DRAFTABLE, DRAFT_STATE , and iteratorSymbol all use the built in backups for when there is no Symbol in the environment.
4. Add an export similar to setUseProxies called setUseSymbols.

To Reproduce

Environment must be using a Symbol polyfill

enableES5();
setUseProxies(false);

const baseState = {items: []};
const nextState = produce(baseState, (draftState) => {
    draftState.items.push({id: "ID"});
});

Observed behavior

Exception is thrown:

scope.ts:81 Uncaught TypeError: Cannot read property 'type_' of undefined
    at revokeDraft (scope.ts:81)
    at Array.forEach (<anonymous>)
    at revokeScope (scope.ts:63)
    at processResult (finalize.ts:53)
    at Immer.produce (immerClass.ts:117)
    at <anonymous>:6:17

Expected behavior

No exception

Environment

• Immer version: Latest
• Occurs with setUseProxies(true)
• Occurs with setUseProxies(false) (ES5 only)

[mweststrate]

Even polyfilled symbols should have stable identities, which is probably why you fail to reproduce it in code sandbox. I suspect your issue is rather that you have immer twice in your dependency tree, resulting in two living definitions of those symbols.

…

On Tue, 14 Apr 2020, 01:21 Tim, ***@***.***> wrote: 🐛 Bug Report This bug only occurs when using the ES5 implementation (no proxies) and there is a polyfilled version of Symbol on the window object (in my case this is reproducible both with core-js <https://github.com/zloirock/core-js> and es6-symbol <https://github.com/medikoo/es6-symbol>). I already investigated and found where the root cause is, though I am not sure what the best route forward is (I have several suggestions -- see below) Link to repro Unfortunately, this is VERY hard to reproduce in codesandbox. I tried importing polyfills in codesandbox and overwriting the native implementation, but it was VERY finicky and I couldn't get it to work. Hopefully the root cause explained below is enough to go on 😄 Root Cause The issue occurs because there are checks within the immer codebase like this: if (key === DRAFT_STATE) { // do something } One for instance, is in shallowCopy <https://github.com/immerjs/immer/blob/5a968e8bc51d42e4b9024d47656d0d885613aac2/src/utils/common.ts#L153>. In that case (and other similar ones), the draftState object is passed into each where it will iterate through the keys (including the Symbol key used to store draft state). Since Symbol is polyfilled, the draft state key will be stringified and no longer pass the check for key === DRAFT_STATE, since the Symbol polyfill is not native and cannot be attached to an object without being stringified. The native Symbol produces objects with a special typeof === 'symbol whereas the polyfill produces a plain object that can't follow the rules of Symbol. In the case of shallowCopy, things blow up when the draft state key is not skipped (like the comment says it should <https://github.com/immerjs/immer/blob/5a968e8bc51d42e4b9024d47656d0d885613aac2/src/utils/common.ts#L154> ) Potential Ideas 1. Change checks like this to be key == DRAFT_STATE - Loose equals will work for when key is a string and DRAFT_STATE is an object because DRAFT_STATE stringifies to the key value. - Loose equals also works with the native implementation. 2. Change the check to be typeof DRAFT_STATE === 'symbol' ? key === DRAFT_STATE : key === DRAFTE_STATE.toString() - Safely falls back to a string check if the DRAFT_STATE is not a real symbol. 3. Change hasSymbol <https://github.com/immerjs/immer/blob/0d87fc88e8efffdacbb5db295cb9efd624cd2757/src/utils/env.ts#L4> to be const hasSymbol = typeof Symbol !== "undefined" && typeof Symbol("test") === 'symbol' - Esentially, only native Symbol definitions would be eligible. - This way things like DRAFTABLE <https://github.com/immerjs/immer/blob/0d87fc88e8efffdacbb5db295cb9efd624cd2757/src/utils/env.ts#L31>, DRAFT_STATE <https://github.com/immerjs/immer/blob/0d87fc88e8efffdacbb5db295cb9efd624cd2757/src/utils/env.ts#L35>, and iteratorSymbol <https://github.com/immerjs/immer/blob/0d87fc88e8efffdacbb5db295cb9efd624cd2757/src/utils/env.ts#L39> all use the built in backups for when there is no Symbol in the environment. 4. Add an export similar to setUseProxies called setUseSymbols. To Reproduce Environment must be using a Symbol polyfill enableES5(); setUseProxies(false); const baseState = {items: []}; const nextState = produce(baseState, (draftState) => { draftState.items.push({id: "ID"}); }); Observed behavior Exception is thrown: scope.ts:81 Uncaught TypeError: Cannot read property 'type_' of undefined at revokeDraft (scope.ts:81) at Array.forEach (<anonymous>) at revokeScope (scope.ts:63) at processResult (finalize.ts:53) at Immer.produce (immerClass.ts:117) at <anonymous>:6:17 Expected behavior No exception Environment - *Immer version:* Latest - Occurs with setUseProxies(true) - Occurs with setUseProxies(false) (ES5 only) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#577>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBG4PZC2PO4CLTG7VZTRMOT6ZANCNFSM4MHKRM6A> .

[timothympace]

So after much effort, I was able to bend codesandbox to my will. I think there must be something going on there where node_modules executes in a different environment. Had to dig into the source code to find out what the polyfill was using to feature detect Symbol. Anyways, here is a primitive version reproduced in codesandbox: https://codesandbox.io/s/thirsty-shape-rsvv1?file=/src/index.js

^ that only show cases the root problem. If you want to see the issue in action, I also created a repository to showcase it (the actual immer crash & core-js polyfill behavior):

https://github.com/timothympace/immer-bug-reproduce

If you download that ^, npm install, and npm test, you'll see the two tests I put in there fail.

It comes down to the fact that the Symbol polyfill can't exist as a real Symbol key on objects. It get's stringified when it's used as an object key, which leads to what I mentioned in my first comment.

[timothympace]

In shallowCopy:152, the code uses each to iterate over the draft state. Each uses Reflect.ownKeys under the hood (or even Object.getOwnPropertyNames) to look at the keys. Both those methods will return a stringified version of the Symbol, not the actual Symbol itself. That is why the strict equality fails and the Symbol gets included in the shallow copy.

[mweststrate]

Gotcha! I finally understand the problem, sorry that it dawned so slowly and thanks for the repro! I think solution 1 is neat! Solution 3 would be even nicer, but I am not sure that is fully compatible with iterator protocols if the iterator symbol is created by some polyfill.

Interested in creating a PR?

[timothympace]

Yeah I'd be happy to create a PR. Not sure I understand the potential problem with solution 3 though. Maybe I'm not fully understanding.... I'm suggesting to tighten the restrictions of hasSymbol to only be true when there is a native Symbol implementation present (i.e. typeof Symbol() === 'symbol'). That way, things like DRAFT_STATE` fall back to using plain string values instead of a fake symbol.

Let me know which route you think is best for the project and I'll open a PR 😄

[mweststrate]

@timothympace so the concern here is, if we pick our own symbol (or none at all) for Symbol.iterator, than that would not work with code that uses a Symbol.iterator polyfill, and assume it can for example for of iterate an draft array. For that reason I think 1) is the safer solution, if Symbol.iterator is polyfilled, we will be using that.

But with the same reasoning solution 3 can work, if we make sure to not make the check that strict for Symbol.iterator specifically (that should behave as is). Probably doing that is the clearest solution. (I'd fear that in a future enthusiastic cleanup I or someone else might replace == with ===, or use the symbol is some random other place of the code, and reintroduce this bug)

[aleclarson]

🎉 This issue has been resolved in version 6.0.8 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[aleclarson]

🎉 This issue has been resolved in version 7.0.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀