-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Support tree shaking #1190
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
Comments
Even having an option to import through paths would suffice in my opinion: import List from 'immutable/List'; |
I'd be willing to put in the grunt work to chunk it up. Is there an RFC? |
is any progress here? tree shaking is not working for me still immutable@4.0.0-rc.2 I use only one thing: webpack & babel config{
"entry": [
"/Users/mxtnr/rocket/felix/frontend/projects/admin/src/index"
],
"output": {
"path": "/Users/mxtnr/rocket/felix/public/webpack/admin",
"filename": "[chunkhash].js",
"publicPath": "/webpack/admin/"
},
"plugins": [
new webpack.optimize.UglifyJsPlugin({ sourceMap: true }),
// https://webpack.js.org/guides/migrating/#uglifyjsplugin-minimize-loaders
new webpack.LoaderOptionsPlugin({ minimize: true, sourceMap: true }), // @deprecated
],
"module": {
"rules": [
{
test: /\.js$/,
exclude: /node_modules/,
"use": [
{
"loader": "babel-loader",
"options": {
"presets": [
[
"env",
{
"targets": {
"browsers": [
"last 2 versions"
]
},
"modules": false,
"useBuiltIns": true
}
],
"react",
"stage-2"
],
"cacheDirectory": true
}
}
]
},
// ...
]
},
"target": "web",
"resolve": {
"modules": [
"node_modules",
"/Users/mxtnr/xp/spacebuilder/node_modules"
],
"extensions": [
".js",
".coffee",
".cjsx"
]
},
"resolveLoader": {
"modules": [
"/Users/mxtnr/xp/spacebuilder/node_modules",
"node_modules"
]
},
"devtool": "source-map",
"externals": {
"jquery": "jQuery"
}
} |
any intention to work on this ? |
@alzalabany it seems that there is an |
@wmertens I'm using Create React App (2.1.1) and tree-shaking with immutable js (4.0 RC12) still doesn't seem to work. Why is this issue closed? |
https://github.com/facebook/immutable-js/blob/master/package.json |
I've tested this ( Note that I have also created an webpack issue for this: webpack/webpack#8483.
|
Has tree shaking with immutable js worked for anyone? |
why is this issue closed? |
+1, could we re-open it? Thanks! |
From all I can tell, tree shaking was merged into Check if your version of immutable is supposed to support tree shaking with cat node_modules/immutable/package.json | grep module\" An empty response means that you do not have a tree-shaking version ( In my application, immutable 4 introduces many errors, which other folks have noticed. I'm not sure the best path towards adopting tree-shaking, but unless someone can show me otherwise, I'll be stuck on v3 😢 |
@dawsbot I'm not seeing any improvements in
|
Hi, It's not on our timeline for now. Our focus is really to close all the issues related to 4.0.0 only, and release that. But you are right, tree-shaking is a really required features. I re-open this issue and tag it for later. |
In fact, Can you check and tell us if the issue is fixed ? |
I gave rc15 a shot but it doesn't seem affected. 🤔 I'm using the library heavily, so it's possible I really am using 100% of the code. But it's suspicious that the bundle analyzer shows it as one large block. Some packages like lodash display the individual modules broken out: You can see my bundle analyzer report here: https://fe.soapbox.pub/report.html |
Yeah, my guess is that we haven't separated things out enough to get it to tree shake properly, but I haven't looked into it at all yet. |
Running But having only imported I raised in #1961 some clarification about how this could be addressed. It seems, based on the discussion, that there's a lot of cross-referencing instead of a collection of loosely defined objects and utilities. Untangling these cross-references is probably going to be key to making the package tree-shakeable. |
You might not have called
Immutable isn't a loosely collection of objects, it is a collection api/framework. It's not a collection of helper utilities like e.g. lodash. I don't think rewriting immutable into an unfriendly, unusable utility-based framework is a real solution. The only option I currently see is that the compilation of the base collection (CollectionImpl) maybe results in that code being copied to each collection type. If that is the case, we should and probably can rewrite the internals. |
I see! I was under the impression that fromJS only imports Map and List, and that doing so does not also import OrderedMap, Seq, Range, Hash, etc. What I'm gathering now and in #1961 is, due to method chaining and the ability to convert one data type into another, importing Map and List only is enough to import nearly all of immutable.js. I wasn't using the chaining operations directly, and so I didn't consider those use cases. For me, I was only using When I was part of a team that maintained an internal library, we had a mechanism for allowing the user of that library to specify which "use cases" and "services" were going to be exposed. This is because the library itself was very, very huge, and it was wasteful to import all of it into any service. So, if the User class had the method "acceptUserFriendRequest", but if the user of that library didn't explicitly include the method, that method was not getting imported. How this worked is there was a way to mark the individual methods for which "use case" they belonged to, and its use case had to be specified at initialization time for that method to be included. Of course, that scenario is different, and so maybe that solution isn't applicable - those were backend libraries, they were also internal, it's not really tree-shaking, and the patterns were very strictly enforced to conform to the way the library wanted to be used, etc. Drawing ideas from that though - and this is just a stab in the dark! - I'm wondering if an optional configuration file or package.json setting can be made to whitelist or blacklist specific data structures or class methods, such that if the user specifies a "deny all except whitelist" configuration, then we essentially have "manual tree-shaking", and it's a "do this at your own risk" sort of deal on the part of the user of immutable.js. The default behavior is that immutable.js works as it does now - including all classes and methods that might be needed in the bundle. But there is a compile-time option of choosing to include or exclude specific features of the library. And maybe, through incremental rollouts, the option to exclude only Range (random example) is released first, and then the exclusion options are expanded over time as new features. I think what that would mean is immutable.js still has to work if the user only uses the exposed methods, making use of internal methods as needed to make it work even if it isn't directly exposed in the configuration. But, if due to the exclusion settings, if some part of the library becomes unreachable, then that unreachable part can be tree-shaken. For example, if I did not want to use OrderedSet, then as the user of the library, I can make a design decision to exclude the use of OrderedSet and all methods that produce an OrderedSet in my code. Then the bundle would not need to include OrderedSet at all, and it would be my fault as the user of the library if I tried to use OrderedSet after I specifically excluded it in the configuration. I don't know how simple, easy, or even feasible this idea is, so again, grain of salt! I imagine if immutable.js was not designed from the ground-up to be loosely coupled, then this would need significant internal changes. On the other hand, my intuition is that something like this could introduce tree-shaking without breaking changes, because excluding modules under this scheme is opt-in and not automatic. Either way, at least maybe I can learn something new again by throwing this idea out there! Sorry if it misses the mark though! EDIT: Digesting what you said some more, I see you've already addressed my point before I even made it!
Yep, this makes sense. Now I see your point, I don't think it's possible to remove existing methods from a class with tree-shaking. We'd need the logic to remove methods to execute at runtime, at which point tree-shaking is not applicable anymore. Though I can imagine an additive approach where only the base methods of Map were implemented (like .get, .set, .map, etc) and methods that reference, say, OrderedMap, are implemented as classes of their own with, say, one-level inheritance, which can be tree-shaken away if only the base class needs to be used. Following the throughline of my suggestion, then some kind of configuration would select which one is imported - but I don't know myself how that could be done. Maybe it's not possible. Alternatively, if a "base map" (having only .set, .get, .map, etc) and a "complete map" (having .sort, .toList, etc) were implemented, then they could be imported separately. When the "complete map" is imported, "base map" always comes along. But if only "base map" is ever imported, then the "complete map" is tree-shakeable. |
I do not know if we want to go to this path, but following your idea, a possible solution might be to have a "more" tree-shakeable version of immutable that remove all converters import { Map, fromJS } from 'immutable/base'
// the following code won't work with `base` immutable
Map().toSet() It would require for us to build twice, once without It will require a lot of work, and a lot of nurturing though… I don't know if this is worth it (with this solution, tree-shaking, if possible, is worth it). |
I had the same thought and I think the chance to mix up that import a single time is super high. Maybe having extra NPM packages would be more useful/save. I can see three potentially useful targets:
I am not even sure it would be so difficult to split these up, as CollectionImpl already works like an injection tool. Some methods would need moving around, but it would allow leaving away some stuff. In theory, the library could come with empty implementations for unsupported methods that log access and would warn you about usages that would either break code or break tree-shaking. But I am still not sure this is worth it. For one thing a lot of internal classes are still needed (Seq, Hash, ...). |
We are in the same mood here : I know that tens of bytes can be a lot, but having several packages or a complex system will lost user. I would prefer not make any step in this direction if we do not have a solution that is a no-brainer for the end user. |
That makes sense! For now, I'll go a different route, but will circle back to this if I notice myself needing the optimizations. Beyond a freezer library for objects, having persistent data structures in my corner during my preliminary testing helped smooth out the memory profile and decrease GC cycles (I've gone to using POJOs, and I'm seeing lots of GC now). Problem is, going for something like seamless-immutable, even though it's lighter, it hasn't been updated in years, and Immer is friendlier to an object mutation pattern than immutable.js is. But these could be called premature optimizations, admittedly, though in my particular case adding, swapping, or removing immutable.js is very easy due to using lenses, so in practice it costs very little to change which library provides the PDS (or if PDS is used or not). I totally appreciate that supporting only Map or Set - so basically, exposing only the PDS part - may not necessarily be the most value-adding thing to immutable.js right now. Just wanted to share some of the value that I see personally in separating the base data structures from the additional utilities in a tree-shakeable way. :) |
Hi everybody, After trying things in 5.0.0-beta.1 on that feature (#1888), it did break immmutable. I see some blocking things for tree shaking to work (in order to be useful I mean, not removing 3% size): Collection implementations are centralized
To go further: the "src" dir of immutable is ~420kb (non-gzipped). For the record there is a fork that removed everything except Collections can be convertedWith immutable, you can convert from a collection type to another, for example: Allowing performant tree-shaking would need to either remove those conversions, or adapt two different versions of immutable to handle one choice or the other.
|
I avoid using immutable because it is a huge chunk of code relative to my apps, and for initial page load on mobile, every KB matters. I do get it via third party components, which I then load on-demand only.
However, I noticed that the size is always the same, 57KB minified (using the excellent webpack-bundle-analyzer), which is also the size that it ships with in the npm package.
I doubt that all third party modules use all functionality, so that means that the minification cannot identify unused pieces of code. I build with webpack tree shaking, which works using ES2016 export syntax, first identifying unused exports, not exporting them, and then letting the minification remove any unused code.
So, I think that the immutable library could be smaller if it was also available in an
es
flavor (in the same package, via a package.json key) that exports everything separately.However, that will only help if the code is "chunky", so that if you only use e.g. a
Map
, it will only touch part of the code.Furthermore, the code should not have side effects. I noticed a lot of
createClass
calls, the minifier won't be able to optimize those away because it cannot assume it to have no side effects.Another caveat is that lodash faces the same problem, and the
lodash-es
build is not helping, probably due to the fact that the default export exports everything as an object. Lodash has to resort to a Babel plugin that rewritesimport {foo} from 'lodash'
toimport foo from 'lodash/foo'
, and as a consequence, bothlodash
andlodash-es
get the same build size without tree shaking.TL;DR: If Immutable is "chunky", it can lead to smaller build sizes by tree shaking iif it has an
es
build and it doesn't export everything ondefault
and things likecreateClass
are done at build time. I think/hope.The text was updated successfully, but these errors were encountered: