feat: use esbuild by default for es modules, if detected #625
Conversation
charliegroll
added
the
type: feature
⏱ Benchmark resultsComparing with 53d0281 largeDepsEsbuild: 12.5s⬆️ 2.39% increase vs. 53d0281 Legend
largeDepsZisi: 1m 11s⬆️ 7.46% increase vs. 53d0281 Legend
|
|
|
||
| // Would love to just use await fs.promises.readFile, but it's not available in our version of Node. | ||
| // eslint-disable-next-line node/no-sync | ||
| const srcFileContents = fs.readFileSync(srcFile, { encoding: 'utf8' }) |
There was a problem hiding this comment.
this could probably use some try/catch safety with a default to false to safely maintain the status quo
| @@ -0,0 +1,22 @@ | |||
| const fs = require('fs') | |||
|
|
|||
| const { init, parse } = require('es-module-lexer') | |||
There was a problem hiding this comment.
We already have a JavaScript parser (acorn) as a dependency, which should be able to tell us whether a file uses ESM or not. If it is, I would try to use it before adding a new dependency.
There was a problem hiding this comment.
sure sure, I'll take a look 👍
There was a problem hiding this comment.
I am wondering about the performance difference there considering:
acornis implemented in JavaScript whilees-module-lexeris in WebAssemblyacorndoes lexing+parsing whilees-module-lexerdoes lexing onlyacornparses all possible JavaScript syntax whilees-module-lexeronly looks for imports/exports
The author of es-module-lexer actually claims the following:
For an example of the performance, Angular 1 (720KiB) is fully parsed in 5ms, in comparison to the fastest JS parser, Acorn which takes over 100ms.
There was a problem hiding this comment.
I second @ehmicky 's evaluation - we need a parser and that's just not what es-module-lexer provides.
that being said, es-module-lexer does have a very light footprint... if we want, I can look into how to accomplish this with acorn, but we can't replace acorn with this
zip-it-and-ship-it on cg2/617/DetectEsModules [$] is 📦 v4.20.0 via v16.7.0
❯ du -ha node_modules/es-module-lexer
4.0K node_modules/es-module-lexer/types/lexer.d.ts
4.0K node_modules/es-module-lexer/types
4.0K node_modules/es-module-lexer/LICENSE
4.0K node_modules/es-module-lexer/CHANGELOG.md
12K node_modules/es-module-lexer/dist/lexer.cjs
12K node_modules/es-module-lexer/dist/lexer.js
24K node_modules/es-module-lexer/dist
8.0K node_modules/es-module-lexer/README.md
4.0K node_modules/es-module-lexer/package.json
48K node_modules/es-module-lexer
| @@ -272,16 +272,14 @@ testBundlers('Can use dynamic import() with esbuild', [ESBUILD, ESBUILD_ZISI], a | |||
| await zipNode(t, 'dynamic-import', { opts: { config: { '*': { nodeBundler: bundler } } } }) | |||
| }) | |||
|
|
|||
| testBundlers('Bundling does not crash with dynamic import() with zisi', [DEFAULT], async (bundler, t) => { | |||
| await t.throwsAsync(zipNode(t, 'dynamic-import', { opts: { config: { '*': { nodeBundler: bundler } } } }), { | |||
There was a problem hiding this comment.
this is no longer a reachable flow, since we'll detect imports/exports and default to esbuild
Co-authored-by: ehmicky <ehmicky@users.noreply.github.com>
|
|
There was a problem hiding this comment.
This looks great! I left a few comments, but nothing major.
There are a couple of things that I would like to see before launching this:
-
An idea of performance implications of parsing function files. We have a basic benchmarking tool in the CI, but in addition to that I'd like to take a couple of large sites and compare the bundling times with and without the parsing.
-
I'd like to release this behind a feature flag which, when disabled, should bypass the parsing logic entirely. This will allow us to roll this out gradually to minimise risks and monitor performance implications on real sites. We'd need to add the flag here and then have Netlify Build conditionally sending it based on a LaunchDarkly flag.
I think it might be helpful to also compare the performance with Edit: after discussing, we concluded this would lead to too many false positives. |
|
I ran some preliminary benchmarks using the existing fixtures and got the following: # run.sh
#!/usr/bin/env bash
exec 3>>.delta.largeDepsZisi
exec 4>>.delta.largeDepsEsbuild
exec 5>>.delta.largeDepsZisi.withFlag
exec 6>>.delta.largeDepsEsbuild.withFlag
npm ci --prefix benchmarks/fixtures
node benchmarks/zisi.js >&3
node benchmarks/esbuild.js >&4
NETLIFY_EXPERIMENTAL_DEFAULT_ES_MODULES_TO_ES_BUILD=true node benchmarks/zisi.js >&5
NETLIFY_EXPERIMENTAL_DEFAULT_ES_MODULES_TO_ES_BUILD=true node benchmarks/esbuild.js >&6
|
There was a problem hiding this comment.
This looks great, thanks @charliegroll! I've added super minor comments.
Co-Authored-By: Eduardo Bouças <mail@eduardoboucas.com>
charliegroll
force-pushed
the
cg2/617/DetectEsModules
branch
from
3fec239
to
ff27067
Compare
charliegroll
changed the title
- Summary
It'd be great if we could detect ES Module functions and automagically use esbuild as the bundler, similar to the way Typescript detection works.
- Test plan
TODO(I've added a simple test for POC)- A picture of a cute animal (not mandatory but encouraged)
Closes #617