-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Bundle an es module #1204
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
Bundle an es module #1204
Conversation
I acknowledge there should be a "module" like in the |
package.json
Outdated
@@ -15,17 +15,20 @@ | |||
"url": "https://github.com/facebook/immutable-js/issues" | |||
}, | |||
"main": "dist/immutable.js", | |||
"jsnext:main": "dist/immutable.es.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is no longer recommended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From rollup docs:
Note: Tools such as rollup-plugin-node-resolve and Webpack 2 treat jsnext:main and module interchangeably. They're the same thing, except that module is more likely to become standardised.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jsnext:main
has been deprecated in Rollup: rollup/rollup#969
I think no way tree-shaking, because it has been bundled into a pack. |
Is this something that is being considered? I believe it would make sense to have an es module included in the final package |
I'd hope to. It's only little more than a POF on my part, but I'd like to receive some feedback, especially on adoptability. |
It should work because Webpack will modify bundle exports so that unused (i.e. not imported by some other module) parts of the code are not exported anymore, and then it will be cleaned up by "dead code elimination" after minification with Uglify/Babili (see https://webpack.js.org/guides/tree-shaking/ for details). That's why ES build should contain untranspiled imports/exports – it can be statically analyzed and modified. I've been waiting for modular ImmutableJS so long ❤️ 🙏 |
@deepsweet Webpack can tree-shaking |
@Shyam-Chen theoretically yes, it includes a "module" attribute in its package.json, and uses rollup. |
If I understand correctly, the only 2 practical differences here is using Seems fine to me. |
I'm really curious how well this will work! I noticed code like I will take a look at the effect next week or so. |
@leebyron Are there any instructions on how to use this new es-module? I'm not getting any benefits in build after the update. I haven't modified the way I import immutableJS in the codebase. It could also be something related to my Webpack config. Therefore some light on how to benefit from this would be nice |
@inakianduaga it should be noticeable only with minified builds because it relies on "dead code elimination". have you checked your final |
also you can take a closer look at Immutable.js in your bundle, it should contain special "unused harmony export" comments made by Webpack for stuff you have never used. example. |
@deepsweet Yeah, this was for minified files. I will check the webpack comments on the bundle. BTW, do you have a rough estimate of the reduction in size? I'm using list/map/records and nothing else from the library. I'm also using Typescript which last time I looked recently didn't play so nicely with tree shaking, so that might be another thing |
I haven't checked it by myself yet, but it depends on Immutable.js internals – if list/map/records are using 90% of the entire library then you'll get that 90% in your bundle. And if so then it requires another PR to reduce amount of internal cross-dependencies when it's possible. |
That's about 95% of the library :) - You're not likely to see much reduction |
I just installed |
This PR is aimed to produce an es module build, targeted by bundlers (e.g. rollup & webpack).
This will enable tree shaking on their side, so will fix #1190.