fix(remix-dev/vite, remix-server-runtime): handle criticalCss in an adapter agnostic way
#8076
Conversation
🦋 Changeset detectedLatest commit: 80797d6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
hi-ogawa
changed the title
RemixViteRuntime to handle critcalCss in an adapter agnostic waycritcalCss in an adapter agnostic way by exposing RemixViteRuntime
hi-ogawa
changed the title
critcalCss in an adapter agnostic way by exposing RemixViteRuntimecritcalCss in an adapter agnostic way
hi-ogawa
changed the title
critcalCss in an adapter agnostic waycritcalCss in an adapter agnostic way
hi-ogawa
changed the title
critcalCss in an adapter agnostic waycritcalCss in an adapter agnostic way
There was a problem hiding this comment.
I love that this means consumers don't need to worry about managing the critical CSS for custom servers. I've pushed some refactors, mainly to get rid of the indirection of modifying the server build, but overall this looks good to me.
Since my changes were pretty significant, I'm throwing it to @pcattori to have a look too.
| ) { | ||
| (res.locals as Record<string, any>).__remixDevCriticalCss = | ||
| criticalCss; | ||
| } |
There was a problem hiding this comment.
This is such a nice clean up 🎉
markdalgleish
changed the title
critcalCss in an adapter agnostic waycriticalCss in an adapter agnostic way
| const globalDevServerHooksKey = "__remix_devServerHooks"; | ||
|
|
||
| export function setDevServerHooks(devServerHooks: DevServerHooks) { | ||
| // @ts-expect-error | ||
| globalThis[globalDevServerHooksKey] = devServerHooks; | ||
| } | ||
|
|
||
| export function getDevServerHooks(): DevServerHooks | undefined { | ||
| // @ts-expect-error | ||
| return globalThis[globalDevServerHooksKey]; | ||
| } |
There was a problem hiding this comment.
Nice! I didn't think about exposing get/set API.
If I look at this simplicity, then I wonder if it would also work with just using local let variable like:
let _devServerHooks: DevServerHooks | undefined;
export function setDevServerHooks(devServerHooks: DevServerHooks) {
_devServerHooks = devServerHooks
}
export function getDevServerHooks(): DevServerHooks | undefined {
return _devServerHooks;
}But this might have issues when dual package hazard like scenario (user-code is esm, but remix library is commonjs)?
I was just looking at the sveltekit for how they deal with ssrFixStacktrace and it looks like they have a clever way to pass "vite server thing" to "runtime":
const { set_fix_stack_trace } = await vite.ssrLoadModule(
`${runtime_base}/shared-server.js`
);
set_fix_stack_trace(fix_stack_trace);export let fix_stack_trace = (error) => error?.stack;
export function set_fix_stack_trace(value) {
fix_stack_trace = value;
}I'm still digesting what's happening with this, but it might be possible to avoid globalThis, so I'll investigate further and I can follow up with a separate PR if I find a better way.
markdalgleish
changed the title
criticalCss in an adapter agnostic waycriticalCss in an adapter agnostic way
|
@hi-ogawa Thanks so much for kicking off this PR. This is a massive improvement. |
|
🤖 Hello there, We just published version Thanks! |
|
🤖 Hello there, We just published version Thanks! |
Closes: #7878
(EDIT: virtual module +
globalThisapproach is rewritten with explicit get/set API exposed by@remix-run/server-runtime. The following comment is only about the former approach.)This PR adds
__unstableRemixViteRuntimeto virtualServerBuildso that server-runtime can use it to implement certain vite dev specific features such ascriticalCss(and also my motivation is to use this to exposessrFixStacktrace#8066).The idea is that, by extending
ServerBuild, it allows server-runtime layer to handle certain features instead of adapter layer.Messing with
globalThisis probably not ideal (since this is essentially exposing API from@remix-run/devto@remix-run/server-runtimeviaglobalThis.__unstableRemixViteRuntime), but I saw there are other frameworks making use of similar approach to expose certain dev features through virtual module, for examples:So, I thought this ideas is worth a shot, so I put together a PR. Please let me know if this approach is worth considering. Thanks!
Testing Strategy
Existing tests (namely
vite-css-dev-test.tsandvite-css-dev-express-test.ts) should cover the functionality.