Ramda thinks mobx observable arrays are empty

[andys8]

I noticed there is an issue, when using mobx with other libraries. The concrete example is that ramda thinks observable arrays are empty. This is one example but I guess there are other, similar issues

Sample reproduction: https://stackblitz.com/edit/js-weoxpn

const array0 = observable([]);
const array1 = observable([1, 2, 3]);

const isArrayEmpty0 = isEmpty(array0);
const isArrayEmpty1 = isEmpty(array1);

I'm aware of the pitfalls section and .toJS() as a solution. Because I use ramda a lot I'm thinking about the options we have to improve the behavior? Can there be changed something in mobx? Another solution would be to try to adapt ramda, but it would be hard to argument the change.

This is how ramda checks emptyness: https://github.com/ramda/ramda/blob/v0.25.0/source/empty.js

This is how ramda checks if something is an array: https://github.com/ramda/ramda/blob/v0.25.0/source/internal/_isArray.js#L16

Object.prototype.toString.call(val) will produce [object Object] with mobx, but ramda would want to see [object Array]. Because it's probably a bad idea to try to change the output, it is probably complicated to find a solution to fix this on the mobx side.

[mweststrate]

As the pitfalls section indicates indeed, use .slice() (not the same as toJS, the latter is recursive) to pass an array to an external lib that doesn't support observables.

Once MobX 5 with proxy support has landed this is not strictly needed anymore, assuming your target JS engine supports proxies.

Since this is documented and discussed extensively, closing the issue.

[andys8]

Okay, thanks for the answer.

[urugator]

I wonder whether implementing Symbol.toStringTag would be enough? If that could provide (at least partial) interoperability with any lib which doesn't rely on Array.isArray, but rather on Object.prototype.toString.call(obj) (which seems quite common), I think it's worth considering...

[mweststrate]

Ah, that could help in some cases indeed

…

[andys8]

Sounds interesting. I was hoping for a proposed solution like this.

@mweststrate Worth further investigation? Reopen the issue?

[mweststrate]

Yes feel free to reopen for now. It won't fix the issue in general, but occassionally it might help and it is non impactful, small change I think :) Op ma 9 apr. 2018 om 17:10 schreef Andy <[email protected]>:

…

Sounds interesting. I was hoping for a proposed solution like this. @mweststrate <https://github.com/mweststrate> Worth further investigation? Reopen the issue? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1490 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhAKhEIbasIMEf2tlpr5iU6oscfJtks5tm3nmgaJpZM4TMM4i> .

[andys8]

Great. I think you have to reopen it.

[spion]

With the isConcatSpreadable and toStringTag symbols, the problem is, they're not available in IE11, which is also currently the only serious browser that doesn't have proxies.

Does anyone know if modern polyfills patch array methods to make them respect isConcatSpreadable and toStringTag? If they do, then this will fix a whole world of problems. We could just recommend an IE11 polyfill to deal with lodash/rambda/native array footguns.

You could then use lodash array methods with mobx!

[andys8]

The indicator on this site says there is a polyfill available for both of the mentioned methods.

https://cdn.polyfill.io/v2/docs/features/

[mweststrate]

@spion good point. Forgot about that. Again.

@andys8 the problem is not that they are not polyfillable, but the problem is that the browser natively won't pick these up. Which means that using these features would make your app behave slightly different between engines that support these tags, and engines that don't. Which is against the design principles of MobX. (We prefer to have the same bug on all browsers, then having them on some, as the latter is much harder to debug & maintain).

To clarify, Symbol.toStringTag itself can easily be polyfilled, but if Object.prototype.toString doesn't respect it, it doesn't help us further. And as far as I can see, the polyfills don't actively monkey-patch Object.protoype.toString

So, fixed in MobX 5 only it will be it seems...

[spion]

@mweststrate no worries - when I said that, it did not occur to me to check if maybe Object.prototype.toString and Array.prototype.concat are polyfilled / patched to respect the symbols.

Looks like in core.js (babel's polyfill) toStringTag is implemented but isConcatSpreadable isn't fully implemented. Maybe we could add some documentation that for IE11 support those polyfills are required?

Not sure why isConcatSpreadable isn't implemented by the way. It seems that only Array.prototype.concat is using it. Can't imagine any significant performance loss being caused by overriding it...

[urugator]

@@isConcatSpreadable and @@species is implemented in core-js@3 (currently alpha)

[mweststrate]

Released toStringTag support for arrays in 4.2.0. This should solve your problem @andys8. Didn't implement isConcatSpreadable, can be done ones core-js is GA I think

[andys8]

Wow, so great to hear this! Happy to see how the issue actually lead to an improvement in the end. Thanks for your efforts, @mweststrate.

[andys8]

Giving it a try. Ramda's type recognizes observable object's now as Arrays. The change has lead to improvements. The example with isEmpty is now returning false for empty and non-empty arrays.

https://stackblitz.com/edit/js-qymnte

This is due to the implementation in Ramda. An empty instance is generated from the input and compared for equality with the input. The internal implementation of _equals is not trivial. I think this is as far as it get's. Having quirks in one of those libraries to support the other is not a good solution. The cause is, that an observable array is not an array. I'm looking forward to mobx 5 and am curios what changes it will bring :)