embroider-build / embroider

Compiling Ember apps into spec-compliant, modern Javascript.
MIT License
339 stars 137 forks source link

How to lazy load CSS with FastBoot #1049

Open simonihmig opened 2 years ago

simonihmig commented 2 years ago

Somewhat related to #1033, I played with importing CSS (Sass) from JS. This seems to work quite fine with a classic Ember SPA, but not so much when FastBoot comes into play.

First thing I had to do was to use mini-css-extract-plugin instead of style-loader, even in development mode, as style-loader would immediately break FastBoot by using not supported DOM APIs.

Stack trace ``` ReferenceError: document is not defined at insertStyleElement (node_modules/style-loader/dist/runtime/injectStylesIntoStyleTag.js?:93:15) at addStyle (node_modules/style-loader/dist/runtime/injectStylesIntoStyleTag.js?:208:13) at modulesToDom (node_modules/style-loader/dist/runtime/injectStylesIntoStyleTag.js?:81:18) at module.exports (node_modules/style-loader/dist/runtime/injectStylesIntoStyleTag.js?:239:25) at eval (webpack:/app-template/components/lazy-css.css?:16:156) ... ```

Now with using only mini-css-extract-plugin, and with #1048 also applied, loading the index route of a FastBoot powered app that imports CSS from app.js does work correctly! I.e. the imported CSS has a link in the prerendered HTML (with #1045 applied even in the correct place). However when I have for example lazy loaded routes, and those import CSS (in route.js, or some component not previously used), then mini-css-extract-plugin will then (correctly) try to dynamically inject the <link> for that CSS chunk here, however that fails when this runs in the FastBoot VM.

Stack trace ``` ReferenceError: document is not defined at findStylesheet (/var/folders/q3/yzggf_l965zbzps_bk8xr37c0000gn/T/broccoli-34347pIJ0N0jVFeLE/out-155-packager_runner_embroider_webpack/assets/chunk.ab6b31dc622375f2f6d5.js:255:36) at /var/folders/q3/yzggf_l965zbzps_bk8xr37c0000gn/T/broccoli-34347pIJ0N0jVFeLE/out-155-packager_runner_embroider_webpack/assets/chunk.ab6b31dc622375f2f6d5.js:272:17 at new Promise () at loadStylesheet (/var/folders/q3/yzggf_l965zbzps_bk8xr37c0000gn/T/broccoli-34347pIJ0N0jVFeLE/out-155-packager_runner_embroider_webpack/assets/chunk.ab6b31dc622375f2f6d5.js:269:20) at Object.__webpack_require__.f.miniCss (/var/folders/q3/yzggf_l965zbzps_bk8xr37c0000gn/T/broccoli-34347pIJ0N0jVFeLE/out-155-packager_runner_embroider_webpack/assets/chunk.ab6b31dc622375f2f6d5.js:285:58) at /var/folders/q3/yzggf_l965zbzps_bk8xr37c0000gn/T/broccoli-34347pIJ0N0jVFeLE/out-155-packager_runner_embroider_webpack/assets/chunk.ab6b31dc622375f2f6d5.js:134:40 at Array.reduce () at Function.__webpack_require__.e (/var/folders/q3/yzggf_l965zbzps_bk8xr37c0000gn/T/broccoli-34347pIJ0N0jVFeLE/out-155-packager_runner_embroider_webpack/assets/chunk.ab6b31dc622375f2f6d5.js:133:67) at Object.load (webpack://embroider-sass-app/./assets/embroider-sass-app.js?:65:32) at PrivateRouter.eval [as getRoute] (webpack://embroider-sass-app/./node_modules/@embroider/router/index.js?:95:23) ```

Usually we have learned to guard usage of unsupported browser APIs that could run in FastBoot with something like if (typeof FastBoot === 'undefined'), but this won't work here:

Here is a reproduction app: The index page loads fine, but /lazy shows the error described above.

simonihmig commented 2 years ago

@ef4 this was the issue I brought up in the "office hours" last week. You mentioned a bug that needed to be fixed, I guess this was #1109, right? So I updated my reproduction branch (see link above) with the latest release, but the problem still exists. And I would have been surprised if not, as the changes we need don't belong to the build process, but the runtime: when FastBoot renders a route (here /lazy) that includes a lazy import of CSS, the rendered HTML should include that <link> to it. So basically the webpack plugin's (mini-css-extract-plugin) runtime code needs to be fully executed to insert that link into our DOM. But again, that does not work in FastBoot, due to... no DOM.

I am not sure I completely understood your second part of the answer: I think you suggested that we can add just enough APIs to FastBoot (SimpleDOM to be precise) so these things can run in FastBoot. Not sure though if you wanted to have enough of the DOM APIs available so the webpack plugin's runtime code works as it is (which seems hard to do, when powerful APIs like document.querySelector are used), or if you wanted to replace the runtime code of the plugin with an "emberified" version of it that works in FastBoot (and maybe only very minimal API extensions to SimpleDOM)?

For this particular plugin, there is the runtime option to completely disable adding runtime code, but I don't see an easy way to replace it (do we need to write our own plugin then here?).

However even if that works somehow, I think it still shows that the DX story here is not as simple as it used to be before, like "install ember-cli-foo and you're good". The alternative story of "just install webpack-plugin-foo and add it to your config" does not work, when you have to find non-trivial solutions to workaround the limitations of FastBoot. This is what I referred to in the session as some kind of fundamental conflict (between FastBoot and webpack plugins).

I wanted to write this down here, also to bring my own thoughts into order, but we can totally pick this up again in the next/a future embroider Team session if you prefer the higher bandwidth channel! cc @rwjblue

ef4 commented 2 years ago

You're right, #1109 was only one part of the problem. There are a few other things.

First: In apps with fastboot, @embroider/webpack should automatically use MiniCssExtractPlugin even in development (as opposed to style-loader, which is fine in development for apps that don't have fastboot). ember-auto-import already has this policy and embroider should do the same. This would be a great well-scoped PR.

Second: the actual lazy CSS handling can go two directions.

Third: I am currently investigating another bug where a production fastboot build can incorrectly try to do runtime loading of a browser-only chunk. In the build output index.html in production you will see:

<script src="/assets/chunk.123.js" data-fastboot-src="/assets/chunk.456.js"></script>

This is because we've generated optimized browser-only ("chunk.123.js") and fastboot-only ("chunk.456.js") versions of this code. The fastboot server knows to load chunk.456.js and then strip out the data-fastboot-src (which the browser would ignore anyway). But I have a bug reproduction where the fastboot server crashes by trying to load chunk.123.js at runtime.

This isn't CSS specific, but the way it dies looks very similar so could confuse any experimentation, so I'm including it on our complete "make fastboot work easily" todo list.

I am not sure I completely understood your second part of the answer: I think you suggested that we can add just enough APIs to FastBoot (SimpleDOM to be precise) so these things can run in FastBoot.

Yeah, I did suggest that as a possibility too. The actual code changes required to support the subset we'd need would be small, but the more I think about it I think it would be hard to roll out without breaking other things, so on second thought I don't advise pursuing that solution.

This is what I referred to in the session as some kind of fundamental conflict (between FastBoot and webpack plugins).

I don't think this is fundamental, I think this is just embroider fastboot support being less mature than non-embroider fastboot support. @embroider/webpack is supposed to handle all the webpack config we've been discussing, it's just a bug that it doesn't yet.

simonihmig commented 2 years ago

Sorry for the late response...

First: In apps with fastboot, @embroider/webpack should automatically use MiniCssExtractPlugin even in development (as opposed to style-loader, which is fine in development for apps that don't have fastboot).

Yes, I did that already in my playground...

ember-auto-import already has this policy and embroider should do the same. This would be a great well-scoped PR.

Ok, I'll look into this!

Second: the actual lazy CSS handling can go two directions.

I'm not really convinced of Option One. It's better certainly than breaking FastBoot, but I would have expected Option Two as a user. Or at least whatever the implementation is (custom loader or what else), that when CSS is imported from a lazy chunk, then it should also get loaded lazily - both in the browser and in FastBoot (i.e. in the later case it should only be added to the prerendered HTML when that route actually needs it).

This would be relatively easy and honestly I would be shocked if anybody can show a measurable performance downside

For many cases you are probably right. Like for our company website where I was originally experimenting with that stuff, it wouldn't matter much indeed. But you can also think of other cases, where it does. Like you have a landing page for anonymous users (ok, if you are really worried about perf, serving a SPA for a landing page is probably not the best idea, but anyways...), and a big fat app behind a login. So you wouldn't want the landing page to eagerly load all CSS for pages, that the user might not even have any access to.

I don't think this is fundamental, I think this is just embroider fastboot support being less mature than non-embroider fastboot support.

Hm, still not entirely convinced here. So yes, the use cases that we have in mind to work out of the box should do just that, work. So when lazy loading of CSS is considered a thing we explicitly want to support, then the current state is "just a bug".

But what about use cases that we don't really have on our radar? Like someone wants to run wasm code, and the webpack loader happens to need to use some DOM APIs. Idk if it does, this is just a contrived example. So in the browser this would just work, while adding FastBoot would break things. Because all webpack plugins can safely assume they have full access to DOM, while FastBoot does not provide this. Would FastBoot come with a fully spec-compliant DOM replacement like jsdom, then we wouldn't have any problem at all, right? So this is what I referred to as "fundamental". We cannot guarantee that any random webpack plugin works, because we cannot fulfill the assumption they all make that they have access to DOM.