Closed iamursky closed 4 years ago
Yes, I'm planning on doing this, but it'll likely be after other features land first.
Note to self: There's a tree shaking size comparison here that could be interesting: https://github.com/evanw/esbuild/issues/86#issuecomment-624454372.
Another example: https://github.com/evanw/esbuild/issues/81#issuecomment-633185889
Thanks for these links! These are very helpful.
I have tree shaking basically implemented, and about to be released. It's currently in a branch as I test it and prepare it for release. This release is going to be about correctness, not minimal bundle sizes. It introduces the tree shaking data structure but doesn't yet handle the side-effect annotations you mention in https://github.com/evanw/esbuild/issues/86#issuecomment-625923444. That will have to come in subsequent releases.
Is there information anywhere about how the "sideEffects"
hint is supposed to work? Using it to remove the whole module if it's unreferenced makes sense to me. I'm wondering if it's supposed to be used to infer something about the top-level statements in the file not having side-effects too.
Just as one would find the defacto pure annotations test examples in the uglify-js tree where it was first implemented, you'd have to look at the webpack source tree for "sideEffects" test cases. The last link in https://github.com/evanw/esbuild/issues/86#issuecomment-625923444 was actually the official documentation for the latter.
Yeah I'll have to study the test cases, and do some reverse engineering to answer some of my questions. I read the documentation but it was too high-level and didn't really tell me enough information to implement it.
I just released version 0.4.0 which has the preliminary tree shaking support described above. Please try it out and let me know if you encounter any correctness issues. As I said above, esbuild isn't respecting annotations yet so the size is likely still going to be bigger than other bundlers. Right now tree shaking is only based on which top-level statements reference which other top-level statements. I'll do another push later on to improve on bundle sizes.
The "sideEffects"
package hint is relatively simple - it is either true, false, or an array of files within the module with side effects - implying that all files not listed in the array are considered to be side-effect-free (whether actually the case or not). Here's an array example:
"sideEffects": true
and "sideEffects": []
are effectively the same - all files within the package are considered to have side effects.
If a side effect free file within a module is imported and no symbols are used from it, then the import can be dropped altogether. But if any exported symbol is imported from a side effect free file then its top level statements with side effects must be retained, and only its unreferenced symbols without side effects may be dropped. To use a garbage collector analogy, any used exports or top level statements with side effects are considered to be roots.
Here's an example with additional documentation: https://github.com/webpack/webpack/tree/master/examples/side-effects
The following was observed behavior from rollup's implementation:
"sideEffects": true
and"sideEffects": []
are effectively the same - all files within the package are considered to have side effects.
but it doesn't seem to be logically consistent with
"sideEffects": [ "file_not_present_in_package.js" ]
which makes the entire package behave like "sideEffects": false
.
I don't know whether webpack has the same behavior as rollup with an empty "sideEffects"
array. If they are in disagreement, use webpack's behavior.
🎉🎉🎉 Preliminary tree shaking support Great job @evanw
Upgrading serverless-esbuild
If a side effect free file within a module is imported and no symbols are used from it, then the import can be dropped altogether.
Yes, that model makes sense to me. That's the trivial way to do things safely.
To use a garbage collector analogy, any used exports or top level statements with side effects are considered to be roots.
This is what I've implemented in the latest release.
But if any exported symbol is imported from a side effect free file then its top level statements with side effects must be retained, and only its unreferenced symbols without side effects may be dropped.
Since code is JavaScript, it's impossible to determine what is guaranteed to be free of side effects outside of a whitelist of a few types of expressions such as literals. I was wondering if the "sideEffects": false
annotation somehow biases the compiler's determination of what is side-effect free. For example, you might have code like this:
export function foo() {}
foo.bar = 123
It's impossible to determine based on this code whether the second statement has side effects or not. Someone might have done this somewhere before this was evaluated:
Object.defineProperty(Function.prototype, 'bar', {
set() {
sideEffects()
}
})
And if the second statement is retained then the first line must also be retained. I'm wondering if "sideEffects": false
is supposed to mean something like "trust me these property accesses don't have side effects". Otherwise tree shaking isn't going to do that much in the benchmarks you referenced.
I could see an alternative algorithm where, if "sideEffects": false
is present, code with side-effects can form a connected component in the top-level statement graph but connected components could still be dropped if there are no references into the connected component from the outside (also similar to garbage collection). But referencing something in the connected component would pull in the whole connected component. So referencing foo
would also pull in the assignment to foo.bar
.
I was wondering if the
"sideEffects": false
annotation somehow biases the compiler's determination of what is side-effect free.
The side effect determination ought be the same whether or not "sideEffects": false
is in effect and an export from a given file is used.
Be aware that Rollup and Terser can be fooled by pathological cases of Object.defineProperty
pretty easily even with normal code without a "sideEffects"
package hint. See also: https://github.com/rollup/rollup/issues/2219.
For people following along, there's a thread relevant to tree shaking in #86. Let's merge that thread into this one.
I have cloned https://github.com/mischnic/tree-shaking-example into https://github.com/evanw/tree-shaking-example and added support for esbuild
. I also integrated the ideas around correctness verification from #86 and attempted to fix some of the correctness issues with Parcel and Rollup. Bundles that behave incorrectly are invalid, and their sizes no longer count. There are still some Parcel and Rollup bugs that I couldn't fix.
I just released esbuild 0.4.6 with support for sideEffects
. Here is the latest run of those benchmarks including all esbuild tree shaking progress so far:
file size error
------------------------- -------- --------------------------------------
rollup/lodash-es 18.3kb
parcel/lodash-es 18.8kb
webpack/lodash-es 20.6kb
esbuild-0.4.6/lodash-es 21.3kb
esbuild-0.4.2/lodash-es 91.4kb
esbuild-0.3.9/lodash-es 92.0kb
rollup/lodash 69.2kb
esbuild-0.3.9/lodash 70.4kb
esbuild-0.4.2/lodash 70.4kb
esbuild-0.4.6/lodash 70.4kb
webpack/lodash 72.2kb
parcel/lodash !!!!!!!! Attempted to require "buffer"
rollup/rxjs 9.3kb
parcel/rxjs 9.8kb
webpack/rxjs 11.0kb
esbuild-0.4.2/rxjs 86.8kb
esbuild-0.4.6/rxjs 86.8kb
esbuild-0.3.9/rxjs 113.2kb
parcel/react-icons 9.6kb
rollup/react-icons 9.7kb
webpack/react-icons 10.2kb
esbuild-0.4.2/react-icons 1241.9kb
esbuild-0.4.6/react-icons 1241.9kb
esbuild-0.3.9/react-icons !!!!!!!! Missing file: react-icons.js
rollup/remeda 2.0kb
parcel/remeda 2.1kb
webpack/remeda 2.9kb
esbuild-0.4.2/remeda 7.6kb
esbuild-0.4.6/remeda 7.6kb
esbuild-0.3.9/remeda 13.0kb
rollup/ramda 6.3kb
parcel/ramda 6.5kb
webpack/ramda 7.3kb
esbuild-0.4.2/ramda 44.7kb
esbuild-0.4.6/ramda 44.7kb
esbuild-0.3.9/ramda 47.7kb
parcel/ramdaBabel 6.9kb
esbuild-0.4.2/ramdaBabel 7.8kb
esbuild-0.4.6/ramdaBabel 7.8kb
esbuild-0.3.9/ramdaBabel 7.9kb
webpack/ramdaBabel 8.5kb
rollup/ramdaBabel !!!!!!!! Attempted to require "ramda/src/range"
rollup/rambda 0.8kb
esbuild-0.4.2/rambda 1.8kb
esbuild-0.4.6/rambda 1.8kb
webpack/rambda 2.5kb
parcel/rambda 9.8kb
esbuild-0.3.9/rambda 12.1kb
rollup/rambdax 6.2kb
esbuild-0.4.2/rambdax 7.8kb
esbuild-0.4.6/rambdax 7.8kb
webpack/rambdax 8.4kb
parcel/rambdax 22.5kb
esbuild-0.3.9/rambdax 34.9kb
The jump from 0.3.9 to 0.4.2 is the initial tree shaking support for ES6 imports and exports. This improves various benchmarks including rxjs
(113.2kb => 86.8kb) and rambda
(12.1kb => 1.8kb). This also fixes a bug with re-exports in react-icons
.
The jump from 0.4.2 to 0.4.6 is the newly-released support for the sideEffects
hint. This improves the lodash-es
benchmark (91.4kb => 21.3kb) but doesn't affect the other benchmarks at all.
Follow-up work is support for __PURE__
hints, which are not supported yet. Big thanks to @kzc for all of the help so far.
Please update to the latest versions of parcel, rollup, webpack, babel and related packages:
--- a/package.json
+++ b/package.json
@@ -20,22 +20,21 @@
"rxjs": "^6.5.2"
},
"devDependencies": {
- "@babel/core": "^7.4.4",
- "@babel/preset-env": "^7.4.4",
- "babel-loader": "^8.0.6",
+ "@babel/core": "7.10.2",
+ "@babel/preset-env": "7.10.1",
+ "babel-loader": "8.1.0",
"console.table": "^0.10.0",
"filesize": "^4.1.2",
"npm-run-all": "^4.1.5",
- "nyc": "^14.1.1",
- "parcel-bundler": "^1.12.3",
- "rollup": "^1.12.1",
- "rollup-plugin-babel": "^4.3.2",
- "rollup-plugin-commonjs": "^10.0.0",
- "rollup-plugin-node-resolve": "^5.0.0",
- "rollup-plugin-terser": "^4.0.4",
- "terser-webpack-plugin": "^1.2.4",
- "webpack": "^4.31.0",
- "webpack-cli": "^3.3.2"
+ "parcel-bundler": "1.12.4",
+ "rollup": "2.13.1",
+ "rollup-plugin-babel": "4.4.0",
+ "rollup-pluginutils": "2.8.2",
+ "rollup-plugin-commonjs": "10.1.0",
+ "rollup-plugin-node-resolve": "5.2.0",
+ "rollup-plugin-terser": "6.1.0",
+ "webpack": "4.43.0",
+ "webpack-cli": "3.3.11"
},
"scripts": {
"clean": "rm -rf .cache parcel rollup webpack esbuild-*",
@@ -55,7 +54,7 @@
"esbuild:rambda": "LIB='rambda' node esbuild.js",
"esbuild:ramda": "LIB='ramda' node esbuild.js",
"esbuild:ramdaBabel": "LIB='ramdaBabel' node esbuild.js",
- "parcel:lodash": "parcel build src/lodash.js --experimental-scope-hoisting -d parcel",
+ "parcel:lodash": "parcel build src/lodash.js -d parcel",
"parcel:lodash-es": "parcel build src/lodash-es.js --experimental-scope-hoisting -d parcel",
"parcel:rxjs": "parcel build src/rxjs.js --experimental-scope-hoisting -d parcel",
"parcel:react-icons": "parcel build src/react-icons.js --experimental-scope-hoisting -d parcel",
rollup@2.x
sourcemap configuration changed in the last major. This is the simplest workaround:
--- a/rollup.config.js
+++ b/rollup.config.js
@@ -27,5 +27,4 @@ export default [
}
},
- sourcemap: false,
toplevel: true
})
parcel/lodash !!!!!!!! Attempted to require "buffer"
Might be a parcel scope hoisting bug. Disabling --experimental-scope-hoisting
works for that test. See package.json diff above.
rollup/lodash 70.6kb
webpack/lodash 72.2kb
parcel/lodash 92.6kb
@mischnic could provide a better answer.
rollup/ramdaBabel !!!!!!!! Attempted to require "ramda/src/range"
CJS require/exports and ES import/export constructs should not be mixed within the same source file. This works for all bundlers:
--- a/src/ramdaBabel.js
+++ b/src/ramdaBabel.js
@@ -13,4 +13,4 @@ function fn(x) {
)(x);
}
-export const answer = fn(10).join(',');
+exports.answer = fn(10).join(',');
rollup/ramdaBabel 6.5kb
parcel/ramdaBabel 6.9kb
webpack/ramdaBabel 8.5kb
Thanks for the corrections. I just applied them to https://github.com/evanw/tree-shaking-example. I also fixed some bugs I recently found with sideEffects
and did another release. The main bug was that the hint was being ignored inside nested directories, which was a big oversight. That obviously had a huge effect on bundle sizes.
Here's the latest run with version 0.4.7 of esbuild:
file size error
------------------- -------- -----
rollup/lodash-es 18.0kb
parcel/lodash-es 18.8kb
webpack/lodash-es 20.6kb
esbuild/lodash-es 21.3kb
esbuild/lodash 70.4kb
rollup/lodash 70.6kb
webpack/lodash 71.9kb
parcel/lodash 92.6kb
esbuild/rxjs 9.7kb
parcel/rxjs 9.8kb
rollup/rxjs 10.1kb
webpack/rxjs 10.3kb
parcel/react-icons 9.6kb
rollup/react-icons 9.8kb
webpack/react-icons 10.0kb
esbuild/react-icons 1241.9kb
rollup/remeda 2.2kb
esbuild/remeda 2.3kb
parcel/remeda 2.3kb
webpack/remeda 3.1kb
rollup/ramda 6.4kb
parcel/ramda 6.5kb
esbuild/ramda 6.7kb
webpack/ramda 7.3kb
rollup/ramdaBabel 6.5kb
parcel/ramdaBabel 6.9kb
esbuild/ramdaBabel 7.7kb
webpack/ramdaBabel 8.5kb
rollup/rambda 3.7kb
esbuild/rambda 4.1kb
webpack/rambda 4.6kb
parcel/rambda 15.0kb
rollup/rambdax 4.9kb
esbuild/rambdax 7.0kb
webpack/rambdax 7.6kb
parcel/rambdax 25.0kb
The bundle size from esbuild is now competitive with every benchmark except for react-icons
. Tree shaking for that one appears to rely on unsafe transformations because of assignments to the displayName
property, so this is not related to the __PURE__
comment feature. If the assignments to displayName
are removed, esbuild generates the smallest bundle size for the react-icons
benchmark. So I'm going to consider this issue on tree shaking complete.
Tree shaking for that one appears to rely on unsafe transformations because of assignments to the displayName property
It's legit to alter an export property. There's nothing unsafe about that:
export var FaBeer = function (props) {
...
};
FaBeer.displayName = "FaBeer";
The unreferenced icons can safely be dropped.
This is what I was posting about above: https://github.com/evanw/esbuild/issues/50#issuecomment-634312506. Those assignments may have side effects. For example, this code may have been run before that code was executed:
Object.defineProperty(Function.prototype, 'displayName', {
set() {
sideEffects()
}
})
Object.defineProperty(Function.prototype, 'displayName'
No bundler or minifier would assume builtins would be altered with pathological setters. esbuild can safely assume the same.
Library or application code is fundamentally different than polyfills. Generally polyfills are not run through bundlers, and if so they would be run without code optimization.
Might be a parcel scope hoisting bug
I wouldn't trust Parcel 1's tree shaking. Parcel 2 works correctly here.
Looks like Parcel 2 produces a bigger bundle for "remeda" than Parcel 1, I'll have to look into that.
I've added another testcase for a known Parcel deopt (https://github.com/parcel-bundler/parcel/issues/4565), where esbuild does very well (even compared to rollup): https://github.com/mischnic/tree-shaking-example (and updated to Parcel 2)
@mischnic - Thanks for your insight and adding the new test case. I thought I had upgraded all the bundlers but I didn't realize that parcel v2 hasn't been officially released yet.
webpack/material-ui 86.5kb
esbuild/material-ui 88.8kb
rollup/material-ui 218.7kb
parcel/material-ui 452.5kb
I wonder what happened to rollup/react-icons between https://github.com/evanw/tree-shaking-example:
parcel/react-icons 9.6kb
rollup/react-icons 9.8kb
webpack/react-icons 10.0kb
esbuild/react-icons 1241.9kb
and the newly revised https://github.com/mischnic/tree-shaking-example:
parcel/react-icons 9.4kb
webpack/react-icons 10.0kb
rollup/react-icons 24.4kb
esbuild/react-icons 1241.9kb
It may be due to the addition of the rollup namedExport for "react-is" to support the material-ui test case.
Edit: I see that rollup-plugin-node-polyfills was also added to the Rollup config.
Does react offer an ES version of their library? This UMD/CJS bundling is less than ideal.
/cc @lukastaegert
Edit: I see that rollup-plugin-node-polyfills was also added to the Rollup config.
That was causing it. I've change the config to only run that plugin for material-ui.
Strange about needing rollup-plugin-node-polyfills for the Rollup material-ui test case. Here's a comparable material-ui example app that doesn't require it to build: https://github.com/evanw/esbuild/issues/81#issuecomment-633185889
Strange about needing rollup-plugin-node-polyfills for the Rollup material-ui test case
ReactDOMServer.renderToString
requires the stream
module (to test if the bundle actually works without a DOM)
In an effort to make an apples to apples comparison, do the other bundlers need a node polyfill for the material-ui test case to operate on a NodeJS target?
Parcel and Webpack include them automatically by default
Parcel and Webpack include them automatically by default
esbuild doesn't, afaik.
Actually, the browser
version of react-dom/server
doesn't import stream
and throws with "The streaming API is not available in the browser, use instead: ..." in the methods that would return a stream.
Rollup doesn't respect node_modules/react-dom/package.json#browser
and bundles the Node version.
Edit: I've fixed the Rollup config, now Parcel is the only outlier in that benchmark.
Thanks again @mischnic.
I was unaware of this rollup plugin functionality:
plugins: [
- resolve(),
+ resolve({
+ browser: true
+ }),
Parcel and esbuild reducing configuration complexity is certainly the way to go. In my opinion a lot of Rollup's plugins should be built in and automatically configured.
FYI: support for tree shaking of /* @__PURE__ */
comments is present as of version 0.5.22. This annotation is also automatically added to all JSX elements. See the release notes for details.
Nice.
Other reacty things could have pure annotations as well - see https://github.com/babel/babel/pull/11428.
Side note... it would be useful if esbuild --bundle
were to support stdin entry points. For example:
$ echo 'import R from "react";R.cloneElement(<div>abc</div>);' | esbuild --loader=jsx --minify --bundle
error: Invalid transform flag: "--bundle"
1 error
Rollup supports bundling from stdin:
echo 'import R from "react";R.cloneElement(<div>abc</div>);' | rollup -p sucrase='{transforms:["jsx"]}' -p commonjs -p node-resolve --silent
All imports would be relative to the current working directory when the entry point is stdin.
Off topic, but somewhat related... react doesn't appear to have "sideEffects": false
in its package.json file. The library would always be included - even if not referenced. I guess it's not an issue for its users.
Side note... it would be useful if esbuild --bundle were to support stdin entry points. All imports would be relative to the current working directory when the entry point is stdin.
This works in esbuild as of version 0.6.1.
I am not sure but it looks like Realms proposal could potentially help with ckecking if
class A {}
A.x = 1;
is side-effect free, e.g. when entire bundle is wrapped in a Realm.
A contemporary option would be Object.freeze(Object.prototype)
etc.
Or you could add a flag, something like --consider-global-objects-frozen
and tree-shake such cases only when it is set.
Hey folks, any idea how to debug tree shaking? Is there a verbose mode that could tell when tree-shaking is disabled (detected a side-effect, incorrectly set main fields etc.)? It's quite difficult to tell if tree shaking is working ok or not on a massive codebase, and which module or package maybe faulty for dead code remaining. For instance, I end up with many unused variables in my bundled script. I use Esbuild via Tsup if that changes anything.
Hi @eric-burel try this https://github.com/floydspace/serverless-esbuild/issues/121#issuecomment-1221535325
@evanw, are you planning to add tree shaking to exclude unused code from bundles?