Mobx: Dealing with the MobX array != real array footgun

[spion]

I know its already listed on the pitfalls page, but I think this is a serious enough issue that it needs more effort.

Its really easy to accidentally use a MobX array in a context where it wont do what you expect. The most common one of those is the concat operation: even with TypeScript on board its easy to concat a MobX array to a regular array and get a nested array.

The second most common source of issues are lodash methods. Not sure why they fail - perhaps because they use Array.isArray?

Depending on the code, the issue might propagate quite far before it manifests as an actual bug - mostly due to JavaScripts wonderful array of implicit conversions. Arrays of strings (and arrays-of-arrays-of-strings) are especially vulnerable, since element[k] returns a string in both cases, equality comparison does a lot of coercion, and there are quite a few shared methods (concat, slice, etc).

I run into this issue approximately once per 250 LOC of MobX models. Every time its very hard to track down due to propagation. I'm fairly certain that once other developers join in, this is going to be a recurring nightmare.

I don't have a definite idea on how to fix this (that is, without proxies). Here are the first two ideas that come to mind:

• Have a development-mode add-on that will monkey-patch common array methods to warn/throw on common errors. For example, I think it would be safe to throw whenever you try to concat a MobX array onto a regular array, or warn whenever Array.isArray is used on a MobX array.

• Faking it, by creating an actual arrays and then assigning __proto__ to something else. Sounds like something that has been tried already?

[mweststrate]

Hi @spion,

Yes this is one of the most awkward things by far in MobX indeed. Some random thoughts

1. Indeed, faking to be an array is not possible
2. Proxy support is closer and closer, and is by far the most elegant way to fix this. In fact we could even start adding a observable.proxiedArray() method if needed
3. Rule of thumb is to .slice() your observable array before passing it to another lib, this avoids all the issues and works perfectly as long as the other lib doesn't try to mutate the list
4. I have to admit I barely need to concat observable arrays; In derivations I often filter / map / slice them first, resulting in a plain array. When updating the state, I prefer splicing over concatenating and old and new array, for me it feels cleaner to splice the old one, as it is closer to the 'intend' imho.
5. Monkey patching isArray could be done, but I have to admit I find it quite a scary idea. Not sure what the performance impact is. If it is done only in dev mode, it could be even more dangerous; the production build would behave differently?

[mweststrate]

See also #595

[spion]

@mweststrate regarding the first suggestion, monkey patching would only be done to throw an error or emit a warning. This would only serve as a reminder to developers not to use observable arrays in that way. When you get to non-monkey-patched production with hopefully no warnings/errors, the behaviour would be exactly the same

[mweststrate]

I think this is a cool idea, however need to first add an option to have a distinction between dev / production code like react. That mechanism could also be used to solve #653, and optimizing by skipping many invariant checks. Will open a separate issue for that

[caesarsol]

I agree with @mweststrate point 4: I never use "raw" observable values outside of the state, but only computed values.

This has also an added benefit, because it's a good practice to auto-contain in the state the implementation of the transformation of a value. This avoids duplication and repeated bugged behavior.

@spion do you think that could help on the matter? Could you post a real-case scenario in which you concat?

[spion]

@caesarsol It is a computed value e.g.

@computed get allItems() {
  return _.uniq(subStores.reduce((acc, store) => acc.concat(store.items.map(i => i.name)), []))
}

Due to the the initial value being [], the concat method of the accumulator will not recognize mobx arrays. Now if I try to display this list of all item names, I'm likely to get duplicates. If all of the stores contain one item each, the single-item arrays converted to string will look exactly the same as if they were the item - so the bug manifests as senseless duplicates in the UI in seemingly perfectly looking code.

[caesarsol]

@spion Ah, you are right then, that's a surprising edge case. I probably didn't encounter it before because I was lucky...

I think the best way to get others to avoid it is to give a good rule to follow.

For example, I think the real issue here is the Array.prototype.concat function, which accepts both arrays and values as arguments, silently flattening the arrays into result. Maybe using the spread operator to avoid the ambiguity does help?

acc.concat(...store.items.map(i => i.name))

However, your problem might be better solved by using _.flatMap

_.uniq(_.flatMap(subStores, store => store.items.map(i => i.name)))

[spion]

@caesarsol The lodash solution will also break, since _.flatMap also uses Array.isArray under the cover. The footgun is scarier than one would think! 🙃

My preferred solution would be for TypeScript to support decorators that modify the type of the property... In absence of that, concat and isArray can be monkey-patched.

[caesarsol]

@spion Sorry, you are right... I was pretty sure it would have worked, since mobx arrays are iterable. 😿

I've tried the solution with the spread operator ..., and it seems it's working. We could recommend to use it anytime one uses Array.prototype.concat?

[caesarsol]

Actually reading some more code, it seems lodash flatMap looks for a Symbol.isConcatSpreadable boolean on the iterable, which is not set on mobx arrays but I think could be.

@mweststrate is it possible? I'm not sure about support, being it ES6, and by reading in the official polyfill core-js seems the concat polyfill is an unimplemented feature.

Do you think adding it is a risk, because would change behavior depending on the ES6 support?

[spion]

@caesarsol if we put in isConcatSpreadable, it still wont work in many browsers because they don't support the symbol, so we get inconsistent behaviour, which is arguably worse 😞

[spion]

I just had an idea that might work. If we define a getter for isConcatSpreadable that immediately throws an error, we would at least get that warning in modern browsers (and modern copies of lodash). Best part is that it works without monkey patching. I'm going to try this out.

[use-strict]

What about the sort and reverse methods, which behave differently than their native counterparts? As much as I hate the built-in behavior, changing it just leads to confusion. I understood that there are technical limitations that require a fake-array to be used, but I expect array methods to behave the same as before, especially when using a decorator.

Adding custom methods doesn't seem right either, especially if they might conflict with existing specs. Mentioning it in an obscure paragraph of the documentation doesn't help much either. It's just a recipe for disaster and you can easily shoot yourself in the foot even if you are aware of the issue.

[caesarsol]

@use-strict Well, it's always a class different from an Array!
We use custom classes everywhere, this is just another one, iterable and made to be as compatible as possible to an Array. Do not expect they behave exactly like an Array, keep them explicitly separated, and you'll be good.

@spion Any success with your experiment?

[pmoleri]

@spion the _get_ idea is neat, did it work?
Is there any chance to use the symbol by config? I want to use it in electron where I know the exact browser I'm using, therefore, I wouldn't get inconsistent behavior. Thanks.

[mweststrate]

Closing issue as #1076 will address this

[kovalenko0]

Another solution is to redefine "concat" method of native Array class to throw error when Mobx collection is passed.
Of cource, we only need that in dev mode

[stevematdavies]

Would love to see some CLEAR examples of populating observable arrays with async calls then using the data via the store. I am having huge pains in my native app with mob-x not behaving, but there is almost zero clear example of what I should actually be doing.

Presume I gave a store with an observble array:

@observable myArray = []

then in my componentDidMount in my observer Class I make an api call, and on a resolved promise I
say ... response => { myArray.replace(response.data) .. assuming the payload is a valid array.

The in my render function I reference the array to form a list

myArray.map....

However, its throws an error the list remains still empty, even console logging and doing an empty check yields absolutely no results.

Should I some how be iterating the data and pushing it into the array entry by entry? or what does replace actually achieve here.?

Would really love some insight, the documentation is massively lacking.

[FredyC]

Please, refrain from commenting on old and closed issues as only a few people will see that. Open a new issue with a link to this one instead.