Remove 'internal.ts' export file and make MobX tree-shakeable

[jereklas]

I spent some time this weekend digging into what it would take to make MobX tree-shakeable. I discovered that a large part of the problem is that almost every file in the library contain a circular reference of some kind. You can easily discover the circular reference issue by adding the no-cycle eslint rule.

I started the effort and would say that I'm roughly half way through removing the circular references and feel quite confident that removal of the circular references would eliminate the need for the internal.ts export file. Based on the comment on why that file exists, that alone makes this feel like a worth while endeavor, while also having the benefit of making the library tree-shakeable to an extent (a lot more effort would have to go into this if someone was really prioritizing optimal tree-shaking).

Having said all of that, my question is this. Would this effort be something that would be of interest to everyone? I'd much rather contribute it than fork MobX, but this refactor is touching every single file in the package. I've also done a few things as part of the current refactor that I would consider a breaking change, and as such, this would most likely need to be part of a major version increment. Thoughts?

[mweststrate]

As quick a heads up; don't waste time on that, I tried, it makes things worse. Will try to explain later why, when not AFK

…

On Mon, 17 Jul 2023, 07:55 jereklas, ***@***.***> wrote: I spent some time this weekend digging into what would it take to make MobX tree-shakable. As part of that I discovered that a large part of the problem is that there's basically not a single file in the library that doesn't contain a circular reference of some kind to something else. You can easily discover the circular dependency issue by adding the no-cycle <https://github.com/import-js/eslint-plugin-import/blob/HEAD/docs/rules/no-cycle.md> eslint rule. I started the effort and would say that I'm roughly 2/3rds of the way through refactoring it to remove all of the circular references. I'm quite confident that removing the circular references will eliminate the need for the internal.ts export file, and you would never need to fight trying to control the order in which the files import one another. It would also make the library tree-shakeable to some extent (a lot more effort would have to go into this if someone was really prioritizing tree-shaking optimiaztion). Having said all of that, my question is this. Would this be something worth continuing and to contribute back to MobX or should I continue with a fork? I would like to contribute it back, but this refactor is touching every single file in the MobX package. There are also a few things I see that would be "breaking" changes, and likely would mean this change might need to be a major version increment. Thoughts? — Reply to this email directly, view it on GitHub <#3725>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBFXCWZOL6CTB6ARSB3XQTHWFANCNFSM6AAAAAA2MOOG24> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[mweststrate]

In short, the internal pattern is the only way where you can grapple with circular dependencies and have them executed in a deterministic, easily controllable way. Without it, any arbitrary addition or removal of an import statement anywhere, can break the runtime behavior by wrongly ordering declarations. I blogged about it in the past here: https://medium.com/visual-development/how-to-fix-nasty-circular-dependency-issues-once-and-for-all-in-javascript-typescript-a04c987cf0de

On the other hand, for treeshaking it shouldn't make any difference, unless you are treeshaking on a per file basis. But most modern tooling should do that at the declaration level, on which this structure has afaik no influence.

[jereklas]

First of all, thanks for the super quick responses! I saw those this morning and after reading the blog post decided I would limit myself to 2 hours to see if I could finish what I started with the circular references. I was able to finish unwinding everything but stopped there.

There’s a chance that it all fails like you have described, but I was close enough to having something to prototype against that I felt it was worth pushing forward a little more. I probably won’t be able to do anything more on this for a couple of days, but will report back once I’ve had a chance to try building/testing it

[jereklas]

So here are the initial results. In both cases I was using vite (esbuild+rollup) for bundling a plain typescript file. I did hit a weird issue where it wouldn't tree shake when bundling a React app + MobX, but that doesn't really have any bearing on the results, it's just something to figure out and understand. These sizes are based on no minification/compression.

example using the following imports	MobX 6.10.0 Size	My half-baked modifications
import { makeObservable, observable, reaction }	144.65kb	127.58kb
import { createAtom, reaction }	144.28kb	42.87 kb

There is pretty massive improvement (~100kb) when we're looking at using parts of the core library that are not as tightly coupled. But even when using more common imports there was still a decent chunk of code removed (~20kb). This was just time spent trying to unwind things, I wouldn't be surprised if with some effort there could be big gains with respect to tree shaking of the more commonly used portions of the library.

It's also fair to point out that this doesn't use other bundlers, where I'm sure the results would vary. I have limited experience with webpack and didn't want to spend time learning how to set it up for a preliminary test. These results though seem to indicate there is potential value in continuing.

[mweststrate]

Interesting! Are the results purely from reordering, or did you also make code changes? As import cycles should in theory should be tree-shakeable as well (Imagine putting all MobX code in a single concatenate file with no imports, then internal.ts would only have had influence on the order of the items in the file, but beyond that the file should be exactly the same with both mechanisms). Given the difference, it might be that we forgot to mark most modules / methods as side-effect free?

[jereklas]

I changed every import that I could that was just for type information to be import type {…} from “” as those are ignored/removed from the end product due to them only being for type information. I then also reworked how the isXxx predicates were being done so that they weren’t using the prototypes so that several of those methods checking for object types could exist without any dependency on their prototypes. Everything else was just moving things around in files (and changing file locations in a lot of cases) so that there weren’t any import cycles. The cycles are main cause I see for the need for the internal.ts file pattern

[jereklas]

I’m heading to work now, but i think the issue is more nuanced than having things marked as side effect free? I’ll add more of my thoughts later when I have time