Bundling! 馃 #36
Bundling! 馃 #36
Conversation
matthewp
force-pushed
the
dynamic-import-build
branch
from
e04c841
to
eae58cd
Compare
src/build.ts
Outdated
|
|
||
| if(astroConfig.public) { | ||
| const pub = astroConfig.public; | ||
| for await(const filename of recurseFiles(pub)) { |
There was a problem hiding this comment.
I think overall the correct behavior is to simply copy files from the /public directory as-is, as you鈥榲e done here. But I wanted to highlight that .scss files end up in the final build in /public when we don鈥榯 need them.
But this begs the question: if we ignore Sass, what else do we ignore? I think it鈥榮 better behavior to not try and treeshake a user鈥檚 assets (e.g. they may just want to have download links or serve files publicly). But that said, what if we ignored Sass files, and only Sass files for now? There鈥榮 not really ever a scenario where these need to be public, IMO.
There was a problem hiding this comment.
I'd like to just be very explicit about zero building going on in the public/ directory, and every file getting copied over 1:1. If you put a Sass file in the public directory, you must want it hosted for some reason. Otherwise, move them into astro (ex: you can create your own astro/styles folder for external styles that need to be built)
There was a problem hiding this comment.
The main reason that this is copying over public is that dynamic components mean you can't statically analyze the pages to determine what assets are used.
Agree on treating public files as just bytes.
src/build.ts
Outdated
| const rel = pathRelative(pub.pathname, filename.pathname); | ||
| const outUrl = new URL('./' + rel, dist); | ||
|
|
||
| const bytes = await readFile(filename); |
There was a problem hiding this comment.
Just out of curiosity, if we wanted to do some optimization like image optimization in the future, how hard would it be to provide a hook into this? Or is that something to just worry about later?
There was a problem hiding this comment.
I think that this process is very much an MVP one and I'm not sure what a final version will look like. This is doing some stuff that presumably we'd prefer Snowpack do, but that would take a bit of work to refactor snowpack to allow. So extensibility was not really thought about at all at this time. So if you need to add something, just add whereever.
| ] | ||
| } | ||
|
|
||
| const build = await rollup(inputOptions); |
There was a problem hiding this comment.
@FredKSchott can you remember why we decided to use Rollup for the demo rather than Esbuild? One downside to rollup is that you don't get minification.
We can get minification via an esbuild.transform() call, if we'd like.
The main reasons, from my experience building esbuild into snowpack:
- esbuild won't let you overwrite existing files. If we're bundling files in-place, it's a pain to get working.
- esbuild's tree-shaking support is still young and not very well developed. Ditto for esbuild itself, in a lot of ways.
- esbuild's "metafile" uses these relative paths that are also a pain to get working, and since esbuild doesn't allow overwriting files you're forced to use the metafile to re-create the bundle output yourself.
Snowpack can get away with this because you can just reach for Rollup/Webpack when you need it. But assuming that this isn't pluggable in astro, that isn't available to us.
There was a problem hiding this comment.
In my mind this file is mostly a stopgap until we can improve snowpack and use that directly. So I don't have a preference here really, it might just be easier to use terser directly at this point. Thanks!
There was a problem hiding this comment.
+1. Yea the goal (and why Snowpack bet on esbuild) is that the project will eventually get there.
There was a problem hiding this comment.
Going to do minification in a follow-up PR.
No description provided.