Mobx: Why isn't mobx 5 compiled to ES6 only?

[spion]

Hi!

I am wondering, why does the MobX 5 module default to a downlevel ES5 emit? It doesn't make sense to me: if you want to use MobX 5 the browser has to support proxies, which in turn means that there is no reason to set the target to ES5.

I'm asking because I noticed that I'm getting RangeError stack blow-outs with MobX 5 that I didn't get wiht MobX 4. Upon investigation, the culprit seems to be propagateMaybeChanged:

https://github.com/mobxjs/mobx/blob/a5cb65926621266716f8d11d716bf337e60dc3c1/src/core/observable.ts#L216-L231

If I change this function to use a for..of loop, the stack blowout goes away (there is a reduction of at least 30% in function calls, probably 50%), however I noticed the comment above:

https://github.com/mobxjs/mobx/blob/a5cb65926621266716f8d11d716bf337e60dc3c1/src/core/observable.ts#L185

So I'm considering whether to send a pull request that will change the target for MobX 5 to be ES6 with no ES5 option, as well as update the loop to a for..of one to get more stack depth.

[FredyC]

As I said on gitter, there are still browsers which do not fully support ES6. It really depends on what features are used in the code and what is coverage of those in relevant browsers.

It's curious though why are you getting RangeError. We are using MobX 5 in two production apps for like half a year and haven't seen such an error once. Any chance you are doing something strange in your code? :)

[spion]

I do have very long computed chains, as well as methods to avoid making them too long - however it appears that i need to change the method as the limit of the chain has been significantly reduced with mobx 5

Here is a sample to experiment: https://codesandbox.io/s/2ubhx

[schibrikov]

there are still browsers which do not fully support ES6. It really depends on what features are used in the code and what is coverage of those in relevant browsers.

I think @spion meant that mobx is using Proxies anyway, which aren't polyfillable. And browsers that supports them, support ES6 (and most likely more modern stuff) as well.
So there is absolutely no reason to transpile down to ES5, it won't work anyway.

[FredyC]

I do understand what he meant, but even environments who do support Proxies can be lacking some of ES6 features. I don't really know which one is used in MobX code, but it's likely to be safe indeed.

[mweststrate]

Shipping ES5 in npm modules is the only thing that doesn't blow up the eco system. It is not just to make the browser happy, but many other tools in the chain highly depend on dependencies to be compiled down in my experience. (Trust me, I tried) Things might have changed in the mean time. But I don't trust the JS eco system too much yet to be responding to such radical changes to well :lollipop:

[mweststrate]

It could be an interesting experiment to make the change and publish it under a separate tag for a while on npm

[spion]

Fair enough. What about the mobx.module.js version only then? It would seem that any tool that processes that would have to have an es6-compatible parser...

edit: Actually. I will try these out in a separate npm package and report back.

[mweststrate]

I tried that in the past and that assumption turned out to be dead wrong🙈

Op do 6 jun. 2019 20:34 schreef Gorgi Kosev [e-mail]:

Fair enough. What about the mobx.module.js version only then? It would
seem that any tool that processes that would have to have an es6-compatible
parser...

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/mobxjs/mobx/issues/1977?email_source=notifications&email_token=AAN4NBB6XSDFE466GEXUFHTPZFKEZA5CNFSM4HQGPAEKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXDYK2A#issuecomment-499615080,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAN4NBCJ7QZSUZTGFOOPYG3PZFKEZANCNFSM4HQGPAEA
.

[FredyC]

All considered, is it actually worth it? I mean most of the other modules still do compile to ES5. Getting MobX on the other side is a drop in the puddle in my opinion.

What would be gained out of it in the end? Smaller bundle size? How much? Would it be faster when using native code for some features? That needs a benchmark.

So yea, I wouldn't really bother in the end.

[spion]

The for..of loop means ~ 2x longer chains of computed values are possible without a stack blowout.

[FredyC]

Or we can refactor for..of to something less problematic if that's only concern :) To be honest, I haven't used for..of a single time in any of my apps. Array.from(obj.keys()).forEach(...) works just fine, even though it's slightly more verbose.

[spion]

In that portion of the code, any implementation that uses higher order functions has additional call stack overhead of 2 functions per computed

[FredyC]

So? Do you have benchmarks that it's an actual issue or are you guessing? :) Sorry, I don't want to be a buzz killer here, I am just trying to discuss worthiness of such proposal.

Although in the end, we could always have a separate ES6 bundle sitting in mobx-react/es6.js and whoever would dare, they could use it.

[spion]

Sure, here is a demo:

• MobX 4: https://codesandbox.io/s/2ubhx
• MobX 5: https://codesandbox.io/s/whkgv

I stop MobX 4 at depth 10K although it can go a bit further. MobX 5 stops at 5K

If you remove the first element increment both implementations can go indefinitely due to computed caching

I can switch to a for..of loop in that part of the code, publish with target: es6 and I'll probably get 10K again. I've already done this in our production example.

We'll probably have to switch that part of the code to yads although the datastructure isn't ready; however, the stack behavior is definitely worse for MobX 5 compared to MobX 4.

[FredyC]

Hah, just noticed that we actually have ES6 build available ... https://unpkg.com/[email protected]/lib/mobx.es6.js

I've tried it in your example and it doesn't seem to be any different. Perhaps there is some other culprit? About which for..of specifically are you talking about? I don't know that much details here.

[spion]

In the first comment, the code link I quoted contains a forEach loop - specifically the following line in propagateMaybeChanged

observable.observers.forEach(d => {

It seems that for..of compiles to significantly slower ES5 code in TypeScript according to the comment in that same file

// Ideally we use for..of here, but the downcompiled version is really slow... 

so a forEach based loop is used instead.

That's why the ES6 build you tried has the same problem too. A forEach loop always compiles to a forEach loop. The original TS code needs to be changed to for..of and a separate ES6 bundle would need to be created.

[FredyC]

Ah got it.

So if we change the source code to use for..of in that specific spot, it would work great in ES6 build, but might be slower ES5 build? That doesn't sound like a good plan. It would require some conditional bundling through env variables I suppose. Perhaps you can try for a PR?

[spion]

I can also try a conditional compilation PR, yes... although I wanted to avoid adding extra complexity :grinning:. Give me a few days.

[mweststrate]

Closing for inactivity now. Since this is quite a big change, I think it needs a champion (assignee) before reopening it and continuing it.

[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.