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

test(remix-dev/vite): test custom server.hmr.server vite config #8095

Closed
wants to merge 7 commits into from
Closed

test(remix-dev/vite): test custom server.hmr.server vite config #8095

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
68a6495
fix(remix-dev/vite): fix websocket double `handleUpgrade` error with …
hi-ogawa Nov 22, 2023
2f8c248
chore: minor cleanup
hi-ogawa Nov 22, 2023
a8f6cb0
Merge branch 'dev' into fix-vite-child-vite-server-double-handleUpgra…
hi-ogawa Nov 22, 2023
27e1666
test: test custom `hmr.server` config
hi-ogawa Nov 22, 2023
f5ad741
chore: lint
hi-ogawa Nov 22, 2023
ae0ebf6
chore: changeset
hi-ogawa Nov 22, 2023
1ac18c4
Merge branch 'dev' into fix-vite-child-vite-server-double-handleUpgra…
hi-ogawa Nov 23, 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
5 changes: 5 additions & 0 deletions .changeset/swift-yaks-worry.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@remix-run/dev": patch
---

Fix websocket error when using custom vite hmr server config
29 changes: 18 additions & 11 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 @@ -15,7 +15,6 @@ test.describe("Vite CSS dev (Express server)", () => {

test.beforeAll(async () => {
let port = await getPort();
let hmrPort = await getPort();
projectDir = await createFixtureProject({
compiler: "vite",
files: {
Expand All @@ -29,11 +28,6 @@ test.describe("Vite CSS dev (Express server)", () => {
import { vanillaExtractPlugin } from "@vanilla-extract/vite-plugin";

export default defineConfig({
server: {
hmr: {
port: ${hmrPort}
}
},
plugins: [
vanillaExtractPlugin(),
remix(),
Expand All @@ -48,15 +42,28 @@ test.describe("Vite CSS dev (Express server)", () => {
import { createRequestHandler } from "@remix-run/express";
import { installGlobals } from "@remix-run/node";
import express from "express";
import http from "node:http";
import { createServer } from "vite";

installGlobals();

const app = express();

const server = http.createServer(app);

// TODO: should support custom server config via "unstable_createViteServer"?
// https://github.com/sapphi-red/vite-setup-catalogue/blob/7ff3dd0850e5f575a01e222342f979bd70f46523/examples/middleware-mode/server.js#L12-L20
let vite =
process.env.NODE_ENV === "production"
? undefined
: await unstable_createViteServer();

const app = express();
? undefined
: await createServer({
server: {
middlewareMode: true,
hmr: {
server,
},
}
})
Comment on lines +54 to +66
Copy link
Contributor Author

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

Choose a reason for hiding this comment

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

I replaced unstable_createViteServer with direct use of vite.createServer since currently unstable_createViteServer doesn't support passing vite config.
It might make sense to tweak the API of unstable_createViteServer (or remove it entirely in favor of direct vite call?). Please let me know if anyone has opinion on this.

Also, FYI, using server.hmr.server option is one of recommended setups from https://github.com/sapphi-red/vite-setup-catalogue/blob/48cde75352005aa1c1780f5eccf022db5619e285/examples/middleware-mode/server.js (explained further in sapphi-red/vite-setup-catalogue#16 and also this repo is linked from vite's documentation https://vitejs.dev/config/server-options.html#server-hmr)

I just noticed there is now basicTemplate which includes custom server setup.
Maybe updating server.mjs there or having a dedicated test case for this custom server scenario might make more sense.
Please let me know if there is a preferred way to test.

Copy link

Choose a reason for hiding this comment

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

I replaced unstable_createViteServer with direct use of vite.createServer since currently unstable_createViteServer doesn't support passing vite config. It might make sense to tweak the API of unstable_createViteServer (or remove it entirely in favor of direct vite call?). Please let me know if anyone has opinion on this.

I think that direct use of vite.createServer is better, because if you have a need to create vite server it's because you need to customize something.

But if you want to have unstable_createViteServer, at least allow to pass config modifications. Current implementation is not doing something special (only setting middlewareMode) so it's not necessary.


if (vite) {
app.use(vite.middlewares);
Expand All @@ -78,7 +85,7 @@ test.describe("Vite CSS dev (Express server)", () => {
);

const port = ${port};
app.listen(port, () => console.log('http://localhost:' + port));
server.listen(port, () => console.log('http://localhost:' + port));
`,
"app/root.tsx": js`
import { Links, Meta, Outlet, Scripts, LiveReload } from "@remix-run/react";
Expand Down