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.

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

fix(vite): remove server exports from resource routes #7828

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
5 changes: 5 additions & 0 deletions .changeset/curvy-wasps-occur.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@remix-run/dev": patch
---

fix server code interop for vite build
54 changes: 53 additions & 1 deletion integration/vite-build-test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
import * as path from "node:path";
import { test, expect } from "@playwright/test";
import shell from "shelljs";
import glob from "glob";

import {
createAppFixture,
Expand Down Expand Up @@ -51,13 +54,25 @@ test.describe("Vite build", () => {
`,
"app/routes/_index.tsx": js`
import { useState, useEffect } from "react";
import { json} from "@remix-run/node";
pcattori marked this conversation as resolved.
Show resolved Hide resolved

import { serverOnly1, serverOnly2 } from "../utils.server";

export const loader = () => {
return json({ serverOnly1 })
}

export const action = () => {
console.log(serverOnly2)
return null
}

export default function() {
const [mounted, setMounted] = useState(false);
useEffect(() => {
setMounted(true);
}, []);

return (
<>
<h2>Index</h2>
Expand All @@ -66,6 +81,24 @@ test.describe("Vite build", () => {
);
}
`,
"app/routes/resource.ts": js`
import { json} from "@remix-run/node";
pcattori marked this conversation as resolved.
Show resolved Hide resolved

import { serverOnly1, serverOnly2 } from "../utils.server";

export const loader = () => {
return json({ serverOnly1 })
}

export const action = () => {
console.log(serverOnly2)
return null
}
`,
"app/utils.server.ts": js`
export const serverOnly1 = "SERVER_ONLY_1"
export const serverOnly2 = "SERVER_ONLY_2"
`,
},
});

Expand All @@ -76,6 +109,25 @@ test.describe("Vite build", () => {
appFixture.close();
});

test("server code is removed from client assets", async () => {
let publicBuildDir = path.join(fixture.projectDir, "public/build");

// detect client asset files
let assetFiles = glob.sync("**/*.@(js|jsx|ts|tsx)", {
cwd: publicBuildDir,
absolute: true,
});

// grep for server-only values in client assets
let result = shell
.grep("-l", /SERVER_ONLY_1|SERVER_ONLY_2/, assetFiles)
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

Copy link
Member

Choose a reason for hiding this comment

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

Should this be simplified to target just "SERVER_ONLY" for both exports rather than numbering them? Makes modifying this test less error prone.

.stdout.trim()
.split("\n")
.filter((line) => line.length > 0);

expect(result).toHaveLength(0);
});

test("server renders matching routes", async () => {
let res = await fixture.requestDocument("/");
expect(res.status).toBe(200);
Expand Down
14 changes: 0 additions & 14 deletions packages/remix-dev/vite/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -632,20 +632,6 @@ export const remixVitePlugin: RemixVitePlugin = (options = {}) => {

let serverExports = ["loader", "action", "headers"];

let routeExports = await getRouteModuleExports(
Copy link
Member

Choose a reason for hiding this comment

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

I like that the route transform doesn't need to run this anymore.

viteChildCompiler,
pluginConfig,
route.file
);

// ignore resource routes that only have server exports
// note: resource routes for fullstack components don't need a `default` export
// but still need their server exports removed
let browserExports = routeExports.filter(
(x) => !serverExports.includes(x)
);
if (browserExports.length === 0) return;

return {
code: removeExports(code, serverExports),
map: null,
Expand Down