Immer: Barfs on numeric-like properties in pre-ES5 environment

[bard]

🐛 Bug Report

I had originally decided not to bother anyone with this because:

• it's a very small % of the general user base (it just happens to be a much bigger % for my project)
• the issue can be worked around
• the environment has many polyfills and most likely the issue is there rather than in immer

Then I noticed that https://github.com/mweststrate/immer/commit/696342030e13d973a61ad56a431299b3e20de5aa mentions 'non-numeric keys', so maybe you already know where this weird behavior comes from and it may be a low-hanging fruit-fix. Plus, it's a great subject for party chats in the appropriate circles (see "Trivia" below).

To Reproduce

On at least one device with pre-ES5 environment ("AppleWebKit/533.1 (KHTML, like Gecko) Version/4.0 Mobile Safari/533.1") setting a property that isn't a number (but _is coerceable to one_) on a draft object, causes a crash.

import 'core-js/features/object/freeze'
import 'core-js/features/object/is-frozen'
import 'core-js/features/function/bind'
import 'core-js/features/promise'
import produce from 'immer'

const initialState = {
  data: {}
}

const newState = produce(initialState, draft => {
  draft.data['123'] = 'foobar' // <-- crash
})

console.log(JSON.stringify(newState))

Result:

E/browser ( 2462): Console: Uncaught TypeError: Cannot read property
 'get' of undefined http://192.168.1.9:1234/repro-immer-numeric-properties-es5.47866f92.js:116

That line translates to:
https://github.com/mweststrate/immer/blob/02fed2ca6de18cc8e8728a4f887fe00c09a3b22b/src/common.js#L62

Changing the key into something that cannot be coerced into a number (say a123 or 123a) works around the problem.

Link to repro

Not very useful I'm afraid unless you have some ancient device sitting around, but here it is anyway: https://github.com/bard/repro-immer-numeric-properties-pre-es5

Environment

• Immer version: 2.1.3
• [x] Occurs with setUseProxies(false) (ES5 only)

User agent: AppleWebKit/533.1 (KHTML, like Gecko) Version/4.0 Mobile Safari/533.1

Trivia

I suspect that we're in the presence of a fine case of Nancy Typing.

[aleclarson]

Interesting. It seems that keys which are coercible to numbers are then stored as numbers in the object's descriptor map. Or maybe Object.getOwnPropertyNames is the culprit?

We could add an undefined check where the crash is occurring, but I don't know if @mweststrate wants to handle edge cases in pre-ES5 environments.

Another option is to override Object.getOwnPropertyDescriptor and make it coerce keys to numbers when possible. This would require no changes to Immer, which is nice.

[mweststrate]

@aleclarson in principle we don't handle such cases; it's a bug in the browser, not in the library, so in general it means that we cant work around them. However, if:

1. A work around is small (code wise)
2. Doesn't impact performance for non affected environments
3. Behaves semantically entirely the same as non affected environments (replacing exceptions with non-throwing deviating behavior is worse, not better)

Then we can be pragmatic and include a work around, clearly commented.

[bard]

Small update: I tried the test case again with a different set of polyfills (es5-shim), same behaviour. I was hoping this would be down to the polyfill but it seems indeed to be due to the engine.

For reference, the absolutely minimum test case is:

import 'core-js/features/object/get-own-property-descriptor'

const obj = {
  '_123': 'bar',
  '123': 'foo',
}

Object.getOwnPropertyDescriptor(obj, '_123')
// => {"value":"bar","writable":true,"enumerable":true,"configurable":true} as expected
Object.getOwnPropertyDescriptor(obj, '123')
// => undefined

Thinking aloud: are there legit cases where a property exists and yet getOwnPropertyDescriptor returns undefined? From a look at the spec it seems not. In that case, a workaround may be limited to:

// in shallowCopy()
  ownKeys(base).forEach(key => {
    if (key === DRAFT_STATE) {
      return // Never copy over draft state.
    }
    const desc = Object.getOwnPropertyDescriptor(base, key)
    if (desc === undefined) { // begin workaround
      // Property exists (we know because we got it from ownKeys()) but 
      // getOwnPropertyDescriptor() doesn't find it. This is probably due to
      // a bug in (some?) ES3 engines with numeric-looking keys.
      Object.defineProperty(clone, key, {
        value: base[key],
        writable: true,
        configurable: true
      })
    } else if (desc.get) { // end workaround

Would that make sense?

[aleclarson]

Does Object.getOwnPropertyDescriptor(obj, 123) return undefined in your test case? Also, what does Object.getOwnPropertyNames(obj) return?

[bard]

Here's the full run:

obj: {"123":"foo","_123":"bar"} 
Object.getOwnPropertyDescriptor(obj, "_123"): {"value":"bar","writable":true,"enumerable":true,"configurable":true}
Object.getOwnPropertyDescriptor(obj, "123"): undefined 
Object.getOwnPropertyDescriptor(obj, 123): undefined 
Object.getOwnPropertyNames(obj): ["123","_123"] 

[aleclarson]

Try this:

const { getOwnPropertyDescriptor, hasOwnProperty } = Object
Object.getOwnPropertyDescriptor = function(obj, key) {
  return getOwnPropertyDescriptor(obj, key) || 
    (hasOwnProperty.call(obj, key)
      ? { value: obj[key], enumerable: true, writable: true, configurable: true } 
      : void 0)
}

[bard]

obj: {"123":"foo","_123":"bar"}
: Object.getOwnPropertyDescriptor(obj, "_123"): {"value":"bar","writable":true,"enumerable":true,"configurable":true} 
: Object.getOwnPropertyDescriptor(obj, "123"): {"value":"foo","enumerable":true,"writable":true,"configurable":true} 
: Object.getOwnPropertyDescriptor(obj, 123): {"value":"foo","enumerable":true,"writable":true,"configurable":true} 
: Object.getOwnPropertyNames(obj): ["123","_123"] 

Brilliant.

So this can be worked around at the application level and possibly upstreamed to polyfill libraries but it needs neither be in official immer nor in an expressly maintained fork. :clap: :clap: :clap:

[mweststrate]

Nice fix Alec!

On Mon, Mar 18, 2019 at 3:54 PM Alec Larson [e-mail]
wrote:

Closed #336 https://github.com/mweststrate/immer/issues/336.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/mweststrate/immer/issues/336#event-2210649400, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABvGhGoo2zQr1q7wuHcyxsO6dLRjtOKjks5vX6iNgaJpZM4b4Unl
.