Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ES Modules + .mjs #37

Closed
TheLarkInn opened this issue Dec 15, 2017 · 15 comments
Closed

ES Modules + .mjs #37

TheLarkInn opened this issue Dec 15, 2017 · 15 comments
Labels

Comments

@TheLarkInn
Copy link

Hi there!! This is really neat! I was wondering if any of us at webpack could help ship this code over using ES Module Syntax?

Can include motivations if needed! πŸ‘¨β€πŸ”¬πŸ”₯

brianleroux added a commit that referenced this issue Dec 16, 2017
@brianleroux
Copy link
Contributor

So we use this lib in Node and the browser currently. We ship two builds. A the default build and an immutable build. The quick experiment moved to esmodules syntax and then builds using babel-plugin-add-module-exports. Its not quite completely working yet because I missed some of the .default somewhere…findable with some time…and that should fix the tests.

However that aside we do have a prelimary finding: builds are 5kb larger. πŸ™€

So, the challenge is can we retain the esmodules and get that number smaller. I'm sure some combo of plugins and transforms could eventually do this. And then downstream users can consume the esmodules src directly though I'm not sure what the benefit of that is yet. Tree shaking won't mean much given this thing basically just exports a constructor function for consumption. (Open to learning I'm wrong about all of this too of course.)

@spencermountain
Copy link
Owner

spencermountain commented Dec 16, 2017

hi Sean! yeah, i agree if it's useful to module-people, we should do it, then browserify-them-away for the package.main build. I haven't kept up with the latest best-practices, but am happy to help/merge whatever you'd like
cheers

@brianleroux
Copy link
Contributor

It occurs to me that if the esmodules src gets published with the prebundled CJS src it’ll mean the on disk size of the package in node_modules will be way over 200KB. Even just the esmodules src is around 130KB. So best case 50KB larger on disk.

Also note this is the equivalent of shipping coffescript source to npm. In the past regarded as an inappropriate way of doing things because it forces downstream consumers to have a build step defined by upstream packages. Maybe this is better now that there’s some agreement on the syntax of esmodules but does mean rewriting to match the .default semantics.

Still open to cracking this nut but I want to remain clinical about our chosen trade offs and why.

@brianleroux
Copy link
Contributor

Update: got it mostly working in the esm branch but tests still failing.

Was pleasantly surprised to see .default thing is just a webpack config option so in this branch we build ./spacetime.js and ./immutable.js exported as commonjs2. Then module field in package.json points at the esmodules src in src/immutable.js (for now).

The on disk node_modules payload is still quite a bit bigger but it'll be interesting to play with this using unpkg and a browser.

@brianleroux
Copy link
Contributor

brianleroux commented Dec 18, 2017

Update!1!! I got it totally working. Tests and lint are passing. I also figured out a sorta-hacky way to support esmodules alongside the cjs source. The tradeoff is a breaking change to the interface. Instead of the library exporting Spacetime as the main and then bundling spacetime/immutable the library now exports an object that looks like this {Spacetime, ImmutableSpacetime}.

And the best news! Entire on disk node_modules payload is smaller at 76KB!!

The build uses Webpack for creating a UMD bundle of spacetime that package.json β†’ main points at and then Rollup for creating an esmodules build package.json β†’ module points at.

I also had to use babel-register to get the the tests working. So its quite a bit of devDeps to make this happen but interesting exercise nonetheless. Webpack, Rollup and Babel. I sort of had Buble in there with Webpack for a bit too. So the size challenge is met but this still feels very brittle! Needs more testing/hardening/eyeballs. πŸ•΅οΈβ€β™€οΈ

@spencermountain
Copy link
Owner

wow! nice work brian. i just checked it out and things look great.

.... just thinking aloud -
it's safe to assume the module version will support all-the-fancy es6 right? i guess we can take liberties with that version, and write things in the most terse way now. Wasn't rollup the one that pre-ran for-loops and stuff? we can push this stuff hard

nice work!

@brianleroux
Copy link
Contributor

Yeah I think so but we'll want to make the CJS build a bit conservative to support, at least, Node 6.10 --- think this last commit fixes that by adding a rollup/babel plugin. Bumps cjs source filesize up to 42KB tho!

Def interested to see if we can get that es.js file smaller. Was thinking ./fns.js and ./methods.js could consolidate into ./methods for example. I removed the package.json import (for now) to see if there's a way to get just the .version string in via plugin somehow.

@spencermountain
Copy link
Owner

punting this refactor for now, as it's a good idea, but it required a breaking top-level api change, for the immutable/mutable stuff, and increased the filesize a bit.
afaik, the browserify exports still works with cjs, but if anyone has ideas for a quick-fix, i'm in.
will leave this open

@brianleroux
Copy link
Contributor

Yes, I agree. It is a bit disappointing but this was a nice experiment and, objectively, esmodules cost/benefit isn't there yet.

@TheLarkInn
Copy link
Author

Wow how did I miss this thread until today. (Sorry been migrating GitHub emails). Let me catch up here. So did i see that modules are a branch? Also, you had found builds are smaller still?

Maybe a tl;dr will catch me up.

@brianleroux
Copy link
Contributor

tl;dr we managed to get the payload of it smaller IF we pre-bundled the esm/cjs source but that gains us little a breaking API change for a marginal payload win and a much more involved set of deps and build gymnastics to support it. kinda not worth imo but we'll keep this PR open and play with it time to time to see if the space matures.

@voxpelli
Copy link

I added #71 as I think one the main benefits of switching to ESM could be to have the timezone data tree-shaked, which could reduce the final size of the code rather drastically I think

@spencermountain
Copy link
Owner

spencermountain commented Nov 23, 2018

been thinking more about this, open to any recommendations.

the advice usually is to use es-modules internally, then babel-them-away for the 'main' build.

but i've seen webpack complaining about main pointing to a built file...

so to avoid breaking any node projects, my gut says main should still point to ./src.
and ./src should be cjs until node supports modules.

but I saw this and thought it was a nice solution:
image

is there a good way to spin our build into a .mjs file, while keeping the various benefits of modules?

This stuff still hurts the brain, but module support is like 80% in caniuse now! People are doing some neat stuff and we should support it.

@spencermountain
Copy link
Owner

hey, i got a .mjs branch working, and made a rollup esm build, feel free to play around with it if anyone's interested.

i could be convinced otherwise, but I think it's probably still too early to merge this. I think I got to the same point as brian did a year ago. Lovely idea, still too weird.

@spencermountain spencermountain changed the title Publish loose ESM src? ES Modules + .mjs Dec 31, 2018
@spencermountain
Copy link
Owner

hey there - v5.1.0 publishes a .mjs build, which is linked as 'module' in the package.json.

... I guess that's it?
thanks for your patience.
props to rollup!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants