test(remix-dev/vite): add Vanilla Extract to CSS tests #8000
Conversation
|
| // set sideEffects for global vanilla extract css | ||
| let packageJson = JSON.parse( | ||
| await fs.readFile( | ||
| path.resolve(__dirname, "helpers", "node-template", "package.json"), | ||
| "utf-8" | ||
| ) | ||
| ); | ||
| packageJson.sideEffects = ["*.css.ts"]; |
There was a problem hiding this comment.
I realized that currently remix's template has sideEffects: false by default:
So, vanilla extract global style won't work out-of-the-box.
However, I think this is a general behavior of vite build and not specific to remix, so probably any caveat should be mentioned on their documentation side, such as https://vanilla-extract.style/documentation/integrations/vite/
| @@ -60,7 +60,7 @@ test("builds deterministically under different paths", async () => { | |||
| }, | |||
| }), | |||
|
|
|||
| "postcss.config.js": js` | |||
| "postcss.config.mjs": js` | |||
There was a problem hiding this comment.
(EDIT: this diff shows postcss.config.mjs, but I put it back to js. Also I'm not sure postcss is even actually loading .mjs file...)
Something puzzling is happening in this seemingly unrelated test.
After installing @vanilla-extract/vite-plugin in integration/package.json, this test fails (not only on CI but it also fails locally on my PC).
✘ [ERROR] require() of ES Module /home/runner/work/remix/remix/.tmp/integration/remix-node-template-s04nobi5qlo/postcss.config.js from /home/runner/work/remix/remix/node_modules/lilconfig/dist/index.js not supported.
My hunch is that, since @vanilla-extract/vite-plugin has dependency on postcss-load-config, the existence of this in the visible node_modules might be affecting some postcss's internal config resolution. But postcss-load-config v3 seems to have issue with esm postcss/postcss-cli#387 (comment).
There was a problem hiding this comment.
I put it back to postcss.config.js and it seems adding postcss-load-config v4 to root package.json fixes this issue 4222ed9 though I'm not sure if I'm doing the right thing...
Please let me know if it should be handled in a different way.
There was a problem hiding this comment.
Great find. I opened a PR to fix this at the source vanilla-extract-css/vanilla-extract#1231
There was a problem hiding this comment.
Thanks!
I don't have much experience with postcss, so I was quite at lost.
I would be happy to test if the fix is released. Please feel free to let me know!
EDIT: I verified it works now without fiddling with postcss-load-config 08bb2ed. Thanks for the fix!
markdalgleish
changed the title
| plugins: [remix()], | ||
| plugins: [ | ||
| vanillaExtractPlugin({ | ||
| emitCssInSsr: true, |
There was a problem hiding this comment.
It surprised me that this is needed. After digging into it a bit, it makes sense that it's needed for our current approach to inlining CSS in dev though, however it's not clear to me why this would be needed for the production build.
I'm happy for this PR to go through as-is for now since it's still capturing the current state and proving that it works. It'd be a good idea to follow this up separately though.
There was a problem hiding this comment.
Thanks for pointing out.
My intuition was also emitCssInSsr should not be necessarily for production build, but I didn't investigate this in detail (actually I haven't even tested turning this off yet...)
On vanilla extract plugin side, I saw it's setting some default behavior for known ssr frameworks, so maybe similar thing could be done for remix on their side.
I'll follow up and will see if there's any appropriate change on either side.
config.plugins.some((plugin) =>
[
'astro:build',
'solid-start-server',
'vite-plugin-qwik',
'vite-plugin-svelte',
].includes(plugin.name),
)There was a problem hiding this comment.
I'm leaning towards enabling emitCssInSsr by default in VE. It feels closer to the way CSS typically works in Vite where it's part of both the server and client graphs. The fact that CSS is missing in the server graph ends up breaking other tools and integrations because this is unexpected. I'm chatting with the rest of the VE team about it.
Adding Remix to this list would definitely be an easy non-breaking fix though.
There was a problem hiding this comment.
I've opened a PR: vanilla-extract-css/vanilla-extract#1239
There was a problem hiding this comment.
Thanks for checking in with VE!
I also experimented with toggling emitCssInSsr and it seems my intuition was wrong about this.
Probably you know this better, but let me share what I found.
When emitCssInSsr: false, the server build vite build --ssr will preserve vanilla javascript runtime code like below:
// in build/index.js
// originally from app/styles-vanilla-local.css.ts
import { setFileScope, endFileScope } from "@vanilla-extract/css/fileScope";
import { style } from "@vanilla-extract/css";
setFileScope("app/styles-vanilla-local.css.ts", "remix-template-remix");
const index = style({
background: "lightblue",
padding: "20px"
});
endFileScope();
// originally from app/routes/_index.tsx
jsx("div", { "data-css-vanilla-local": true, className: index ... })I was thinking emitCssInSsr: false means that it still compiles styles-vanilla-local.css.ts into hashed class name string (in js) and only skips emitting css code, but that doesn't seem to be the case.
There was a problem hiding this comment.
That's a fair misunderstanding. If we revisit the options for the plugin, your comment is worth considering.
There was a problem hiding this comment.
Tests have been updated to use the latest Vanilla Extract Vite plugin where emitCssInSsr is no longer needed: #8063.
Follow up of #7990