Mobx: [5.10.1] Extending Array.prototype – `Cannot read property 'value' of undefined`

[idudinov]

Hi there!

TL;DR: code sample

I have a:

• [x] Issue:
  
  
  
  • [x] Provide error messages including stacktrace
  
  

mobx.module.js:2199 Uncaught TypeError: Cannot read property 'value' of undefined
    at extendObservableObjectWithProperties (mobx.module.js:2199)
    at Function.object (mobx.module.js:542)
    at ObservableValue.deepEnhancer [as enhancer] (mobx.module.js:378)
    at ObservableValue../node_modules/mobx/lib/mobx.module.js.ObservableValue.prepareNewValue (mobx.module.js:747)
    at ObservableObjectAdministration../node_modules/mobx/lib/mobx.module.js.ObservableObjectAdministration.write (mobx.module.js:3871)
    at Test.set [as data] (mobx.module.js:4094)
    at Module../src/test.tsx (test.tsx:34)
    at __webpack_require__ (bootstrap:19)
    at Module../src/index.tsx (index.tsx:4)
    at __webpack_require__ (bootstrap:19)

• [x] Provide a minimal sample reproduction.
  Extending Array.prototype – MobX 5.10.1
• [x] Did you check this issue wasn't filed before?

• [x] Elaborate on your issue. What behavior did you expect?
  Seems like this issue relates to extending Array.prototype. Having such extension is not a must in most project, and even might be not safe, but with Typescript (which I use) I think such a case would be common.

• [x] State the versions of MobX and relevant libraries.
  Crashes on 5.10.1, doesn't crash on 5.9.4.

Thanks!

[FredyC]

• but with Typescript (which I use) I think such a case would be common.

Uh, can you elaborate on how TypeScript makes it more common? :) On the contrary, it could be rather annoying to correctly modify something like that.

@xaviergonz Any chance this broken behavior might be related to your https://github.com/mobxjs/mobx/pull/1964?

@loklaan Or is it possibly related to those Symbol changes? I wasn't really following that.

[idudinov]

Thanks for considering this!

Uh, can you elaborate on how TypeScript makes it more common? :) On the contrary, it could be rather annoying to correctly modify something like that.

I know that prototype extending is bad practice in general, using it in ES is not safe, leads to errors, so for ES users this bug would not be major at all. My point was about Typescript case, where such extending is type-safe, one can always track it, fix it etc., therefore it might be more spread, so there may be more cases when somebody like me encounters this issue.

Typescript example for extending Array.prototype

declare global {
    interface Array<T> {
        someAsync(cond: (v: T, index?: number, arr?: T[]) => Promise<boolean>): Promise<boolean>;
    }
}

export async function someAsync<T>(this: T[], cond: (v: T, index?: number, arr?: T[]) => Promise<boolean>): Promise<boolean> {
    for (let i = 0; i < this.length; ++i) {
        const ok = await cond(this[i], i, this);
        if (ok) {
            return true;
        }
    }

    return false;
}

Array.prototype.someAsync = someAsync;

And now usages of it should be caught and understood by IDE.

That's not a blocker anyways, since one can stick to 5.9.4.

[spion]

The reason why extending prototypes is not safe is mainly because the language implementer might decide to add a method named exactly like yours some day.

edit: TypeScript might also give you a false sense of security - if you forget to run the prototype assignment in an iframe for example, TS could think you have the method but you wouldn't since iframes have their own array constructor (different realm)

[loklaan]

If you're going to extend Array.prototype (I wouldn't recommend it) then you should copy the descriptors found on other proto functions like concat; essentially, make it non-enumerable.

Forgetting to do this means that any kind of array looping would include the extension property/value, which is the core issue here. I'd imagine this unexpected behaviour would break a lot of library & user land code..

(@idudinov thanks for the reproduction!)

@FredyC Yeah it's the cause - in the past we were looping over object props but now we are looping over an array of prop-keys extracted from the object, hence opening us up to issue were folks are adding enumerable properties to the Array.prototype. What do you want to do?

[FredyC]

@loklaan I suppose it depends if it's easy to fix. Otherwise, I don't think we want to overcomplicate code to support an edge case that not many people does these days. Sorry @idudinov, I suppose you will have to find some other way or stick to an older version for a while.

[idudinov]

If you're going to extend Array.prototype (I wouldn't recommend it) then you should copy the descriptors found on other proto functions like concat; essentially, make it non-enumerable.

@loklaan could you please elaborate on that? Am I able to do this in my code?

@FredyC not a problem, I understand.

[FredyC]

@idudinov You have to use defineProperty to set enumurable: false so it won't be seen when looping over it. In contrast, that makes it less elegant to use so you can start refactoring to a more supported way :)

[loklaan]

@idudinov I should have given an implementation example, sorry mate!

@FredyC I'd have to write some tests and give it a try because I'm unsure where we're expecting non-own properties to work (eg. from intentional user-land proto inheritance). I think I have time to put something together this week - could you assign this to me?

[FredyC]

@loklaan Thanks! For some reason, I cannot assign you, not sure why.

[urugator]

I'm unsure where we're expecting non-own properties to work

While the initial implementation used for (let key in properties) and therefore included inherited props, it would fail anyway on:
const descriptor = Object.getOwnPropertyDescriptor(properties, key)!
The fact that descriptor can be undefined is completely ignored.
So imho this was just an oversight of the original implementation and inherited props shouldn't be included and were never supported.

[ilkkao]

Seeing this error with Ember.js + mobx. I believe Ember still extends Array.

[ilkkao]

What Ember does is explained well here: https://guides.emberjs.com/release/configuring-ember/disabling-prototype-extensions/

[loklaan]

@urugator Oh yeah you're totally right, thanks for short-circuiting my exploration 😆

@FredyC No worries! PR is up

@ilkkao If you have the time, could you test #2023 against the error you're encountering for Ember? 🙏

[ilkkao]

@loklaan tested, I don't see this error anymore

[mweststrate]

Copied the tests to 4.13 / 5.13 and they pass indeed.

[urugator]

@mweststrate
Note that we should still change the impl of getPlainObjectKeys(object)
it includes inherited keys
but then the key is used to obtain own descriptor
yielding undefined and later failing on descriptor.value

It's also described in this comment

[mweststrate]

Added a guard that extendObservable doesn't accept inheritance based property objects, as changing the behavior of getPlainObject keys sounds potentially more dangerous (it is also used by other api's to reflect on user input)

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs or questions.