FredKSchott / create-snowpack-app

The all-in-one app template for Snowpack. [moved]
https://www.snowpack.dev
Other
727 stars 96 forks source link

[plugin-webpack] Production builds: No bundle splitting based on `import(...)` directives #152

Closed DorianGrey closed 4 years ago

DorianGrey commented 4 years ago

Hey guys,

I've recently tried snowpack on one of my testing projects to see if it can improve the current workflows for both dev and prod. While the currently used webpack configuration works as intended, it's still a bit ... complex.

You can find the current state here: https://github.com/DorianGrey/react-ts-playground/tree/snowpack-experiment

While the development workflow works great, I'm still struggling with bundling for production using @snowpack/plugin-webpack. Since the "regular" build by snowpack does not even minify anything, I'd not consider it a proper alternative for the more complex production projects I'm working on.

The test app heavily utilizes bundle splitting using import(...) directives, mostly for illustration purposes. With the current setup, the production build produces a build output folder containing:

                                               min.               gzip
static\js\main.78f28f65.js                     401.68 KB          124.16 KB
static\js\vendors~todos.321799ce.js            160.25 KB          43.02 KB
static\js\vendors~parseParamTest.3bdab249.js   27.21 KB           10.23 KB
static\js\testRoute1.5d216bbe.js               7.9 KB             3.69 KB
.\favicon.ico                                  24.26 KB           3.63 KB
.\workbox-df7e707f.js                          7.88 KB (+45 B)    3 KB (+23 B)
static\js\lang-de.ba1e9765.js                  9.68 KB            2.82 KB
static\js\lang-en.b521ec04.js                  8.56 KB            2.73 KB
static\js\todos.162909ea.js                    7.53 KB            2.5 KB
static\js\runtime.d41d8cd9.js                  2.47 KB            1.27 KB
.\service-worker.js                            1.93 KB (+502 B)   1.05 KB (+190 B)
.\index.html                                   1.91 KB            792 B
static\js\parseParamTest.194c898c.js           813 B              430 B
.\manifest.webmanifest                         317 B (=)          220 B (=)

With the default production bundle using @snowpack/plugin-webpack (i.e. without modifying its configuration for webpack), the build results looks like:

λ ls -lR build
build:
total 34
drwxr-xr-x 1 Chris 197609     0 Jun 23 20:08 __snowpack__/
drwxr-xr-x 1 Chris 197609     0 Jun 23 20:08 assets/
-rw-r--r-- 1 Chris 197609 24838 Jun 23 20:08 favicon.ico
-rw-r--r-- 1 Chris 197609  2692 Jun 23 20:08 index.html
drwxr-xr-x 1 Chris 197609     0 Jun 23 20:08 js/
-rw-r--r-- 1 Chris 197609   317 Jun 23 20:08 manifest.webmanifest
-rw-r--r-- 1 Chris 197609    67 Jun 23 20:08 robots.txt

build/__snowpack__:
total 1
-rw-r--r-- 1 Chris 197609 61 Jun 23 20:08 env.js

build/assets:
total 4
-rw-r--r-- 1 Chris 197609 2671 Jun 23 20:08 logo-103b5fa18196d5665a7e12318285c916.svg

build/js:
total 804
-rw-r--r-- 1 Chris 197609 818333 Jun 23 20:08 index.0b97b299f40099117f44.js
-rw-r--r-- 1 Chris 197609   2470 Jun 23 20:08 index.0b97b299f40099117f44.js.LICENSE.txt

While the build result works fine, only a single bundle get created, which is not intended w.r.t. the import(...) directives, and the default behavior for webpack would be to create separate bundles for everything imported via these directives, except if explicitly forced to not do so.

As you can see on the specific commit (https://github.com/DorianGrey/react-ts-playground/commit/7015a54a8d46a76c1291b1d892936595e560ca78), I've played a bit with modifying the webpack configuration, trying to enforce the splitting somehow (there might be some duplications to the default config the plugin uses...), which resulted in at most a split between app code and vendor bundle:

λ ls -lR build
build:
total 50
drwxr-xr-x 1 Chris 197609     0 Jun 23 20:10 __snowpack__/
drwxr-xr-x 1 Chris 197609     0 Jun 23 20:10 assets/
-rw-r--r-- 1 Chris 197609 24838 Jun 23 20:10 favicon.ico
-rw-r--r-- 1 Chris 197609  2700 Jun 23 20:10 index.html
drwxr-xr-x 1 Chris 197609     0 Jun 23 20:10 js/
-rw-r--r-- 1 Chris 197609   317 Jun 23 20:10 manifest.webmanifest
-rw-r--r-- 1 Chris 197609    67 Jun 23 20:10 robots.txt
-rw-r--r-- 1 Chris 197609  1510 Jun 23 20:10 service-worker.js
-rw-r--r-- 1 Chris 197609  8019 Jun 23 20:10 workbox-df7e707f.js

build/__snowpack__:
total 1
-rw-r--r-- 1 Chris 197609 61 Jun 23 20:10 env.js

build/assets:
total 4
-rw-r--r-- 1 Chris 197609 2671 Jun 23 20:10 logo-103b5fa18196d5665a7e12318285c916.svg

build/js:
total 808
-rw-r--r-- 1 Chris 197609  52112 Jun 23 20:10 index.beb0074faa5077bc186e.js
-rw-r--r-- 1 Chris 197609 768462 Jun 23 20:10 vendors~index.3efec98b39cb649f6b0c.js
-rw-r--r-- 1 Chris 197609   2470 Jun 23 20:10 vendors~index.3efec98b39cb649f6b0c.js.LICENSE.txt

However, this output did not even work since only the vendor bundle becomes listed in the resulting HTML file, thus the app never starts up. While that might be solved by using the html-webpack-plugin by myself, there is still no splitting based on the import(...) directives.

I'm not sure if this issue is related to https://github.com/pikapkg/create-snowpack-app/issues/74, since the app currently only bundles a single entrypoint.

I know that bundle splitting for illustration purposes only (and in my case: quite small bundles) might seem a bit odd, but if this does not work for those simple circumstances, I'd bother if it works for a really large app.

Thus: Am I missing something regarding the configuration required to get bundle splitting to work (again) for production builds?

FredKSchott commented 4 years ago

Thanks for filing! I'd love to get this working for you. I played around with this a bit and left some comments in your latest commit: https://github.com/DorianGrey/react-ts-playground/commit/7015a54a8d46a76c1291b1d892936595e560ca78

If you make the two changes that I recommended in that commit, I see code splitting happening:

                      js/1.9b604dd810c27c2af542.js    160 KiB       1  [emitted] [immutable]         
                      js/2.572af098b042089b7ec2.js   8.31 KiB       2  [emitted] [immutable]         
                      js/3.41703e3a4bdfe7bd37f2.js   2.44 KiB       3  [emitted] [immutable]         
                      js/4.7fdaa738b2ce28d63e6e.js    9.5 KiB       4  [emitted] [immutable]         
                      js/5.09795443d9466ebd00c3.js  706 bytes       5  [emitted] [immutable]         
                  js/index.33e851f9969c87ddcee6.js    369 KiB       0  [emitted] [immutable]  [big]  index

This has brought up a great point on our end, which is that we don't show any output after bundling, and you're forced to ls in the build directory to make sense of things. Instead, we should emit some stats for you. I'll take a look at adding that

DorianGrey commented 4 years ago

Thanks, I will look into it.

The formatted output in the first example is caused by a webpack plugin that can also be found in that repository. Feel free to pick up some inspiration if that helps (its mostly an aggregation of features I've seen in other plugins, e.g. displaying both minified and gzip size, and size differences to previous bundle part versions).

FredKSchott commented 4 years ago

awesome, thanks!

Checking out your README, a couple more comments:

DorianGrey commented 4 years ago

I still have to figure that out, I've just disabled loading the service worker for now since I'm still not sure if it is better to integrate its generation and setup using the plugin for webpack or its CLI.

I've tried the suggested changes - and added them in a successive commit on the same branch - and yes, that leads to bundle splitting. But there are still two issues:

  1. The code contains several /* webpackChunkName: "<name for the chunk>" */ that do not seem to be processed (they cause a specific name for the output chunk file). Are those comments removed before handling the result over to webpack? Would be acceptable, though, just curious what causes this.
  2. The bundled app cannot be started - it causes an error regarding regeneratorRuntime, a part usually included by babel (Hint: The yarn serve task serves the output of the build folder on http://localhost:4000, if that helps in inspecting this in any way). IIRC that was why I had the @babel/preset-env preset configured before, roughly equivalently to config on the master branch.
FredKSchott commented 4 years ago

For "stripped comments": Yup, that's a side-effect of our import rewriter, but it's not intentional / could be upgraded to support leaving comments in.

For "The bundled app cannot be started": Since you're targeting browsers that don't support async/await, you'll need to include import "regenerator-runtime/runtime.js"; in your application. The readme has more info here: https://www.npmjs.com/package/regenerator-runtime. It would be great if we could automate this for you since you shouldn't need to add that import to dev as well.

Overall, a lot of the problems you're seeing seem based on your custom browserlist that you've defined in your package.json. If you remove that, everything seems to work fine on my end (no need for the regeneratorRuntime fix, but you will be targeting a slightly more modern browser by default). I'd recommend doing that for now while we work on better polyfill support.

appreciate the help debugging!

FredKSchott commented 4 years ago

Okay, last update: Once we publish the next version of Snowpack + #155 your repo should build & bundle as written, no changes needed. The latest changes pushed to #535 mean that you shouldn't need to add import "regenerator-runtime/runtime.js"; yourself anymore, either (should happen automatically with babel-preset + useBuiltins).

DorianGrey commented 4 years ago

Looking forward to test it!

For the testing project, I could change the browserlist config to whatever I desire, yet it's the same I have to use for some production projects, thus it's helpful to leave it that way to figure out if a library or tool would work almost seamlessly in such projects.

FredKSchott commented 4 years ago

These improvements have now been released in the latest version of the plugin (plus a new release of Snowpack which removes the need for import type.

DorianGrey commented 4 years ago

Just a short note: I've checked for the issues, all of them have been resolved, and things are working fine now. Thanks!