Shopify / quilt

[⚠️ Deprecated] A loosely related set of packages for JavaScript/TypeScript projects at Shopify
MIT License
1.7k stars 220 forks source link

Revise sewing-kit-koa production asset loading order #1652

Open BPScott opened 4 years ago

BPScott commented 4 years ago

Overview

The current asset load ordering in sewing-kit koa feels back to front. It load async assets before synchronous entrypoint assets. I would expect it to load assets the other way around - load synchronous assets from the entrypoint then load any async assets.

I don't believe this will have any affect on JS assets - as webpack marshals the execution order of its bundles so load order doesn't really matter.

For CSS assets however the load order impacts the what selector is applied if two of equal precedence are discovered. Most of the time this isn't an issue thank to us leaning heavily on CSS modules but it can crop up. This gets particularly annoying as this asset order in production does not seem to match what occurs in development, where synchronous CSS is included before async CSS. That gets us into the unfortunate situation where production and development output differ.

Consider an app with a main entrypoint, that imports some css to generate a main.css and a react-async component route (which in turn imports some CSS) that generates a myRoute.js and myRoute.css.

Current loading order:

Expected behaviour


For Slack talk: https://shopify.slack.com/archives/CCNRSKLTH/p1602621868239300

@lemonmade :

I assume I know where you are going with this, but IMO the existing (A) behavior is correct because the main files are typically in charge of holding all the shared dependencies of the routes, which means it needs to go first for JS (load in the JS dependencies like React the route JS needs) and CSS (load in the resets and custom properties the route CSS needs)

err

now I think I might have the current thing wrong :P

JS it gets kind of weird, CSS I think I would expect it to be main then route which I think is the opposite of now

@BPScott:

you're right that A is the current behaviour.

@lemonmade:

yeah for CSS I think that's definitely incorrect

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

BPScott commented 3 years ago

still worth pondering IMO. The css approach is certainly incorrect.

lemonmade commented 2 years ago

I think the JS order is unfortunately required if you want SSR to work — the main bundle needs all the components to be available synchronously, which means any async JS needs to have already been loaded by the time the initial render happens.