-
Notifications
You must be signed in to change notification settings - Fork 915
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
Add builtin optimize/bundle support (powered by esbuild!) #1615
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pikapkg/snowpack/5d62dq1c8 |
fa8be4c
to
352f6c7
Compare
352f6c7
to
f66da96
Compare
Snowpack builds all assets (including npm packages) to ESM JS ahead of time, so we take care of a lot of the ecosystem weirdness before the code ever gets to esbuild. We only need esbuild to bundle valid ESM (all import URLs fully resolved) and CSS. Only the @evanw would love to get your thoughts on this & if you think esbuild is ready for this, stability- & timing-wise. This would land in Snowpack as an experiment at first but the plan would be to fully support in Snowpack v3.0 in a couple of months. I have some feedback from the integration that I'd love to share as well, will create a couple follow up issues on your repo tonight/tomorrow. |
Here's what I'd call out:
Sounds great! Thanks. |
Sounds good to all of the above! Appreciate the response.
I could have clarified: we do allow
Ah right, I saw a mention of that. Will keep an eye out, but yup that's fine for now given the experimental status of things on our end. Webpack & Rollup plugins will still exist for more production-ready use. |
f66da96
to
1555aec
Compare
I have no opinion on the bundling feature itself as I generally prefer the unbundled preloading approach, but I noticed in the preload benchmark you have some gray bars in the network inspector, which probably means you're running into the tcp connection limit and are not on http2. If you run the same test using http2 I think the preload version would match/beat the bundled version (my bet is on beat 🙂). |
you're right! I just spun that up on local, we should replace with some real-world examples (H2/H3 + more files). |
"cacache": "^15.0.0", | ||
"cachedir": "^2.3.0", | ||
"cheerio": "^1.0.0-rc.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loose suggestion: if you‘re only using cheerio
to find specific elements like <script>
or <link>
, hypertag works well for this (and even parses attributes). It could be a faster & much lighter-weight alternative. But if you need cheerio for something heavier in the future, it‘s fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, the only reason I switched it out for cheerio was because we do a lot more with it. You can see in the file we're querying, modifying, adding and removing different elements as we optimize.
snowpack/src/build/optimize.ts
Outdated
/** | ||
* Given any set of user-input as entrypoints, | ||
*/ | ||
async function resolveEntrypoints( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More of a DX observation than anything: for my project, I have src/index.js
. So in my config, I specified entrypoints: ['./src/index.js']
. But it threw a “not found“ error. I guess I have to specify ./_dist_/index.js
instead here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see—it wants HTML entrypoints. Hm, similar thing, though—if I specify ['./public/index.html']
or ['./index.html']
both throw a “not found“ error, so I’m not sure how to use this
Error: ENOENT: no such file or directory, open './public/index.html'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should work with both JS & HTML entrypoints, but it looks like there's some bug here. since the 'auto' scanner returns absolute paths, i guess it needs absolute file paths to the final build/
output files? Either way, will fix to support relative files 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should work with both JS & HTML entrypoints, but it looks like there's some bug here. since the 'auto' scanner returns absolute paths, i guess it needs absolute file paths to the final
build/
output files? Either way, will fix to support relative files 👍
Gotcha. Yeah if I specify .js
I get a “only HTML is supported” message right now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also: need to fix the "seeing modulepreloads when only trying to bundle" issue
In this PR we aren‘t pulling styles out of |
|
||
interface ESBuildMetaInput { | ||
bytes: number; | ||
imports: {path: string}[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this build manifest format 👍🏻 . Seeing the imports and size is really cool
platform: 'browser', | ||
metafile: path.join(config.buildOptions.out, 'build-manifest.json'), | ||
publicPath: config.buildOptions.baseUrl, | ||
minify: config.experiments.optimize!.minify, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note: I’m seeing minify
work for JS and CSS, but not HTML. Is that intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
intentional for this PR, but agreed that we should tackle eventually. My hope is that esbuild supports HTML one day, and then we'd get it for free. But, until then we may want to ship some popular html minifier.
I couldn't find one on NPM that I really liked, @drwpow any favorites that you've used in the past?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really. Fine for waiting to see if esbuild supports it as you said
return allBuildFiles.filter((f) => f.endsWith('.html')); | ||
} | ||
if (Array.isArray(entrypoints)) { | ||
return entrypoints; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT about something like this?
if (Array.isArray(entrypoints)) {
- return entrypoints;
+ return entrypoints.map(file => file.startsWith(path.sep) ? file : path.join(cwd, file))
Basically, use an absolute path if provided by the user, or assume it‘s relative to the project directory (see above comment for more context)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, added handling for relative paths to support both final build directory and source directory.
1555aec
to
80219af
Compare
80219af
to
0dc1e6c
Compare
0dc1e6c
to
76da9d9
Compare
76da9d9
to
a5339f0
Compare
a5339f0
to
d0243a8
Compare
d0243a8
to
2a8e9db
Compare
2a8e9db
to
cea48ab
Compare
Background
Changes
experiments.optimize
config that implements the following options:experiments
scope as a part of v3.0.snowpack build
ResultsBefore (2.9sec load over 3G)
preload: true
(2.3sec load over 3G, no observable impact on build speed)(Notice the waterfall is gone)
bundle: true
(2.0sec load over 3G, no observable impact on build speed)Thoughts on esbuild
Testing
Docs