Skip to content

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

Merged
merged 2 commits into from
Sep 29, 2017
Merged

Bundle an es module #1204

merged 2 commits into from
Sep 29, 2017

Conversation

EnoahNetzach
Copy link
Contributor

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.

@EnoahNetzach
Copy link
Contributor Author

I acknowledge there should be a "module" like in the bower.json, I just wasn't able to find out how to do it..

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",

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.

Copy link
Contributor Author

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.

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

@Shyam-Chen
Copy link

I think no way tree-shaking, because it has been bundled into a pack.

@brantphoto
Copy link

Is this something that is being considered? I believe it would make sense to have an es module included in the final package

@EnoahNetzach
Copy link
Contributor Author

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.

@deepsweet
Copy link

deepsweet commented Aug 8, 2017

I think no way tree-shaking, because it has been bundled into a pack.

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 ❤️ 🙏

@Shyam-Chen
Copy link

@deepsweet Webpack can tree-shaking three.js?

@StevenLangbroek
Copy link

StevenLangbroek commented Aug 9, 2017

@Shyam-Chen theoretically yes, it includes a "module" attribute in its package.json, and uses rollup.

@leebyron leebyron merged commit 43f5d67 into immutable-js:master Sep 29, 2017
@leebyron
Copy link
Collaborator

If I understand correctly, the only 2 practical differences here is using export syntax instead of module.exports = syntax, and this is a non-minified source?

Seems fine to me.

@wmertens
Copy link
Contributor

I'm really curious how well this will work! I noticed code like mixin(...) in Collection.js, any fuction calls that run at module load time will be considered as possible side effect code and can't be tree shaken.

I will take a look at the effect next week or so.

@inakianduaga
Copy link

@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

@deepsweet
Copy link

@inakianduaga it should be noticeable only with minified builds because it relies on "dead code elimination". have you checked your final .min.js files?

@deepsweet
Copy link

deepsweet commented Oct 9, 2017

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.

@inakianduaga
Copy link

inakianduaga commented Oct 9, 2017

@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

@deepsweet
Copy link

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.

@EnoahNetzach EnoahNetzach deleted the es-module branch October 9, 2017 14:23
@leebyron
Copy link
Collaborator

I'm using list/map/records and nothing else from the library.

That's about 95% of the library :) - You're not likely to see much reduction

@DanielRamosAcosta
Copy link

I just installed v4.0.0-rc.9, but dist/immutable.es.js isn't in the node_modules directory, any idea why?

@dawsbot dawsbot mentioned this pull request Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support tree shaking