-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
esbuild-wasm support for JS API #86
Comments
@evanw Here's another one to test against: https://github.com/mischnic/tree-shaking-example Submit a PR to demonstrate how esbuild bundle sizes fare against the other bundlers. |
I hacked esbuild support into a local copy of the tree shaking repo above. Here are the results using esbuild 0.2.3 with the options
It appears that esbuild hasn't implemented dead code elimination yet. Anyway, it's a good test case for the project when it does. |
@evanw In addition to dead code elimination support, esbuild would have to implement the following optimizations to achieve the smaller bundle sizes produced by other bundlers:
|
tree shaking example results with esbuild 0.4.0:
|
I'm not sure how much I trust https://github.com/mischnic/tree-shaking-example. I just started looking through the results and I realized that at least the react-icons example is broken. A Rollup bug causes it to not find It's definitely a useful benchmark, and is much appreciated. But I wish it had some test that the generated code is actually correct. |
Just ran the benchmark on version 0.3.9 vs. 0.4.2:
The tree shaking in v0.4 is definitely an improvement, especially in the |
That's odd. I did not experience a crash. Granted react-icons is a poorly written test, but it still produces a result when run:
If you apply this patch to the react-icons test it prints out a proper name: --- a/src/react-icons.js
+++ b/src/react-icons.js
@@ -1,3 +1,3 @@
import { FaBeer } from 'react-icons/fa';
-console.log(FaBeer);
\ No newline at end of file
+console.log(FaBeer.displayName); results with patch:
sizes of that test with patch:
Update the react-icons test if you like, but the other tests are fine. |
The test doesn't evaluate the bundled code. If you change it to evaluate the code, the Rollup build will crash: diff --git a/src/react-icons.js b/src/react-icons.js
index dce9578..1da9aee 100644
--- a/src/react-icons.js
+++ b/src/react-icons.js
@@ -1,3 +1,3 @@
import { FaBeer } from 'react-icons/fa';
-console.log(FaBeer);
\ No newline at end of file
+console.log(FaBeer());
\ No newline at end of file Results with patch:
|
Well, the original test as written did not need to run that function. :-) To build that test correctly after your modifications Rollup requires some manual configuration to work with the CJS format --- a/rollup.config.js
+++ b/rollup.config.js
@@ -2,6 +2,8 @@ import resolve from "rollup-plugin-node-resolve";
import commonjs from "rollup-plugin-commonjs";
import babel from "rollup-plugin-babel";
import { terser } from "rollup-plugin-terser";
+import react from 'react';
+import reactDom from 'react-dom';
const libName = process.env.LIB;
@@ -14,14 +16,19 @@ export default [
babel({
exclude: "node_modules/**"
}),
- commonjs(),
+ commonjs({
+ include: 'node_modules/**',
+ namedExports: {
+ react: Object.keys(react),
+ 'react-dom': Object.keys(reactDom)
+ }
+ }),
terser({
compress: {
global_defs: {
"process.env.NODE_ENV": "production"
}
},
- sourcemap: false,
toplevel: true
})
],
Edit: The rollup config above is incorrect. Use this one instead. After upgrading every bundler and plugin to their latest versions on npm, you can see that Rollup produces a valid result:
The output of the newly revised react-icons test is not deterministic across bundlers though. I recommend changing it to the following: --- a/src/react-icons.js
+++ b/src/react-icons.js
@@ -1,3 +1,3 @@
import { FaBeer } from 'react-icons/fa';
-console.log(FaBeer);
\ No newline at end of file
+console.log(FaBeer().props.attr.viewBox); so that all bundlers produce consistent verifiable results:
With those changes here's the sizes for the latest versions of bundlers:
|
I took a look at the two smallest rollup results in my last post above.
The problem seems to be related to upgrading to The other rollup results as well as the other bundlers' results are bundled correctly. |
The problem with
Corrected results for
The other rollup results are the same. |
The react-icons test would be better if it showed both the displayName and the SVG data for the particular icon:
|
Let's continue this thread in #50. |
It would be useful to have the JS API work with
esbuild-wasm
without the need for a binaryesbuild
package installed.Manually copying the
lib
directory from anesbuild
install toesbuild-wasm
and adding"main": "lib/main.js"
to itspackage.json
appears to work. This untested patch attempts to automate that task:The text was updated successfully, but these errors were encountered: