Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test(remix-dev/vite): add Vanilla Extract to CSS tests #8000

Merged
merged 9 commits into from
Nov 20, 2023
Merged

test(remix-dev/vite): add Vanilla Extract to CSS tests #8000

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0473682
test(remix-dev/vite): test vanilla extract
hi-ogawa Nov 14, 2023
628963b
test: vite-css-dev-express-test
hi-ogawa Nov 14, 2023
1d63d4a
test: vite-css-build-test
hi-ogawa Nov 14, 2023
ce55c6f
chore: keep "@vanilla-extract/css" version
hi-ogawa Nov 14, 2023
b116d6f
test: workaround deterministic-build-output-test by postcss.config.mjs?
hi-ogawa Nov 14, 2023
4222ed9
chore: move around postcss-load-config ...
hi-ogawa Nov 14, 2023
08bb2ed
chore(deps): update @vanilla-extract/vite-plugin
hi-ogawa Nov 15, 2023
198c3f3
Merge branch 'dev' into test-vite-vanilla-extract-plugin
hi-ogawa Nov 15, 2023
ae5f63a
Merge branch 'dev' into test-vite-vanilla-extract-plugin
markdalgleish Nov 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion integration/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@
"@cloudflare/kv-asset-handler": "^0.3.0",
"@cloudflare/workers-types": "^4.20230518.0",
"@playwright/test": "^1.33.0",
"@vanilla-extract/css": "^1.10.0",
"@vanilla-extract/css": "^1.14.0",
"@vanilla-extract/vite-plugin": "^3.9.0",
"cheerio": "^1.0.0-rc.12",
"cross-env": "^7.0.3",
"cross-spawn": "^7.0.3",
Expand Down
55 changes: 53 additions & 2 deletions integration/vite-css-build-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
import { test, expect } from "@playwright/test";
import fs from "node:fs/promises";
import path from "node:path";
import url from "node:url";

import {
createAppFixture,
Expand All @@ -9,26 +12,44 @@ import {
import type { Fixture, AppFixture } from "./helpers/create-fixture.js";
import { PlaywrightFixture } from "./helpers/playwright-fixture.js";

const __dirname = url.fileURLToPath(new URL(".", import.meta.url));

const TEST_PADDING_VALUE = "20px";

test.describe("Vite CSS build", () => {
let fixture: Fixture;
let appFixture: AppFixture;

test.beforeAll(async () => {
// 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"];
Comment on lines +24 to +31
Copy link
Contributor Author

@hi-ogawa hi-ogawa Nov 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized that currently remix's template has sideEffects: false by default:

remix/templates/unstable-vite/package.json

Line 3 in 1d63d4a

"sideEffects": false,

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/


fixture = await createFixture({
compiler: "vite",
files: {
"package.json": JSON.stringify(packageJson, null, 2),
"remix.config.js": js`
throw new Error("Remix should not access remix.config.js when using Vite");
export default {};
`,
"vite.config.ts": js`
import { defineConfig } from "vite";
import { unstable_vitePlugin as remix } from "@remix-run/dev";
import { vanillaExtractPlugin } from "@vanilla-extract/vite-plugin";
export default defineConfig({
plugins: [remix()],
plugins: [
vanillaExtractPlugin({
emitCssInSsr: true,
Copy link
Member

@markdalgleish markdalgleish Nov 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@hi-ogawa hi-ogawa Nov 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

https://github.com/vanilla-extract-css/vanilla-extract/blob/5c9216ca47769d8b042fe46f9662da6a3dc665b1/packages/vite-plugin/src/index.ts#L76-L88

        config.plugins.some((plugin) =>
          [
            'astro:build',
            'solid-start-server',
            'vite-plugin-qwik',
            'vite-plugin-svelte',
          ].includes(plugin.name),
        )

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opened a PR: vanilla-extract-css/vanilla-extract#1239

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a fair misunderstanding. If we revisit the options for the plugin, your comment is worth considering.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests have been updated to use the latest Vanilla Extract Vite plugin where emitCssInSsr is no longer needed: #8063.

}),
remix(),
],
});
`,
"app/root.tsx": js`
Expand Down Expand Up @@ -69,10 +90,28 @@ test.describe("Vite CSS build", () => {
padding: ${TEST_PADDING_VALUE};
}
`,
"app/routes/_index/styles-vanilla-global.css.ts": js`
import { globalStyle } from "@vanilla-extract/css";
globalStyle(".index_vanilla_global", {
background: "lightgreen",
padding: "${TEST_PADDING_VALUE}",
});
`,
"app/routes/_index/styles-vanilla-local.css.ts": js`
import { style } from "@vanilla-extract/css";
export const index = style({
background: "lightblue",
padding: "${TEST_PADDING_VALUE}",
});
`,
"app/routes/_index/route.tsx": js`
import "./styles-bundled.css";
import linkedStyles from "./styles-linked.css?url";
import cssModulesStyles from "./styles.module.css";
import "./styles-vanilla-global.css";
import * as stylesVanillaLocal from "./styles-vanilla-local.css";
export function links() {
return [{ rel: "stylesheet", href: linkedStyles }];
Expand All @@ -84,7 +123,11 @@ test.describe("Vite CSS build", () => {
<div data-css-modules className={cssModulesStyles.index}>
<div data-css-linked className="index_linked">
<div data-css-bundled className="index_bundled">
<h2>CSS test</h2>
<div data-css-vanilla-global className="index_vanilla_global">
<div data-css-vanilla-local className={stylesVanillaLocal.index}>
<h2>CSS test</h2>
</div>
</div>
</div>
</div>
</div>
Expand Down Expand Up @@ -117,5 +160,13 @@ test.describe("Vite CSS build", () => {
"padding",
TEST_PADDING_VALUE
);
await expect(page.locator("#index [data-css-vanilla-global]")).toHaveCSS(
"padding",
TEST_PADDING_VALUE
);
await expect(page.locator("#index [data-css-vanilla-local]")).toHaveCSS(
"padding",
TEST_PADDING_VALUE
);
});
});
72 changes: 70 additions & 2 deletions integration/vite-css-dev-express-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,20 @@ test.describe("Vite CSS dev (Express server)", () => {
"vite.config.ts": js`
import { defineConfig } from "vite";
import { unstable_vitePlugin as remix } from "@remix-run/dev";
import { vanillaExtractPlugin } from "@vanilla-extract/vite-plugin";

export default defineConfig({
server: {
hmr: {
port: ${hmrPort}
}
},
plugins: [remix()],
plugins: [
vanillaExtractPlugin({
emitCssInSsr: true,
}),
remix(),
],
});
`,
"server.mjs": js`
Expand Down Expand Up @@ -115,10 +121,28 @@ test.describe("Vite CSS dev (Express server)", () => {
padding: ${TEST_PADDING_VALUE};
}
`,
"app/styles-vanilla-global.css.ts": js`
import { createVar, globalStyle } from "@vanilla-extract/css";

globalStyle(".index_vanilla_global", {
background: "lightgreen",
padding: "${TEST_PADDING_VALUE}",
});
`,
"app/styles-vanilla-local.css.ts": js`
import { style } from "@vanilla-extract/css";

export const index = style({
background: "lightblue",
padding: "${TEST_PADDING_VALUE}",
});
`,
"app/routes/_index.tsx": js`
import "../styles-bundled.css";
import linkedStyles from "../styles-linked.css?url";
import cssModulesStyles from "../styles.module.css";
import "../styles-vanilla-global.css";
import * as stylesVanillaLocal from "../styles-vanilla-local.css";

export function links() {
return [{ rel: "stylesheet", href: linkedStyles }];
Expand All @@ -127,10 +151,15 @@ test.describe("Vite CSS dev (Express server)", () => {
export default function IndexRoute() {
return (
<div id="index">
<input />
<div data-css-modules className={cssModulesStyles.index}>
<div data-css-linked className="index_linked">
<div data-css-bundled className="index_bundled">
<h2>CSS test</h2>
<div data-css-vanilla-global className="index_vanilla_global">
<div data-css-vanilla-local className={stylesVanillaLocal.index}>
<h2>CSS test</h2>
</div>
</div>
</div>
</div>
</div>
Expand Down Expand Up @@ -166,6 +195,14 @@ test.describe("Vite CSS dev (Express server)", () => {
"padding",
TEST_PADDING_VALUE
);
await expect(page.locator("#index [data-css-vanilla-global]")).toHaveCSS(
"padding",
TEST_PADDING_VALUE
);
await expect(page.locator("#index [data-css-vanilla-local]")).toHaveCSS(
"padding",
TEST_PADDING_VALUE
);
});
});

Expand Down Expand Up @@ -195,6 +232,11 @@ test.describe("Vite CSS dev (Express server)", () => {
TEST_PADDING_VALUE
);

let input = page.locator("#index input");
await expect(input).toBeVisible();
await input.type("stateful");
await expect(input).toHaveValue("stateful");

let bundledCssContents = await fs.readFile(
path.join(projectDir, "app/styles-bundled.css"),
"utf8"
Expand Down Expand Up @@ -234,6 +276,15 @@ test.describe("Vite CSS dev (Express server)", () => {
"utf8"
);

await editFile(
path.join(projectDir, "app/styles-vanilla-global.css.ts"),
(data) => data.replace(TEST_PADDING_VALUE, UPDATED_TEST_PADDING_VALUE)
);
await editFile(
path.join(projectDir, "app/styles-vanilla-local.css.ts"),
(data) => data.replace(TEST_PADDING_VALUE, UPDATED_TEST_PADDING_VALUE)
);

await expect(page.locator("#index [data-css-modules]")).toHaveCSS(
"padding",
UPDATED_TEST_PADDING_VALUE
Expand All @@ -246,6 +297,23 @@ test.describe("Vite CSS dev (Express server)", () => {
"padding",
UPDATED_TEST_PADDING_VALUE
);
await expect(page.locator("#index [data-css-vanilla-global]")).toHaveCSS(
"padding",
UPDATED_TEST_PADDING_VALUE
);
await expect(page.locator("#index [data-css-vanilla-local]")).toHaveCSS(
"padding",
UPDATED_TEST_PADDING_VALUE
);

await expect(input).toHaveValue("stateful");

expect(pageErrors).toEqual([]);
});
});
});

async function editFile(filepath: string, edit: (content: string) => string) {
let content = await fs.readFile(filepath, "utf-8");
await fs.writeFile(filepath, edit(content));
}
Loading
Loading