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): 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

wants to merge 7 commits into from

Conversation

hi-ogawa
Copy link
Contributor

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

EDIT: The change in this PR is obsolete after #8108 and #8120, but it might be still nice to include tests for custom server.hmr.server option scenario. I renamed the PR title accordingly.

Closes: #7953

In #7953 (comment), @Mordred found that this is a general issue when using custom vite server config.
For the use case of child compiler, I think remix doesn't have to delegate user's vite config and also disabling server.hmr entirely seems appropriate.

Testing Strategy

I modified integration/vite-css-dev-express-test.ts to use this server.hmr.server config.

Also, I created a reproduction with the same patch to verify the fix in https://github.com/hi-ogawa/remix-repro-7953 (or https://stackblitz.com/edit/hi-ogawa-remix-repro-7953-mum9cp?file=server.mjs)
Note that this custom server setup is useful for https server as well. You can see this use case here hi-ogawa/test-remix-vite-express-https#1.

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Nov 22, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 1ac18c4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
@remix-run/dev Patch
create-remix Patch
remix Patch
@remix-run/architect Patch
@remix-run/cloudflare Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/css-bundle Patch
@remix-run/deno Patch
@remix-run/eslint-config Patch
@remix-run/express Patch
@remix-run/node Patch
@remix-run/react Patch
@remix-run/serve Patch
@remix-run/server-runtime Patch
@remix-run/testing Patch

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 added a commit to hi-ogawa/test-remix-vite-express-https that referenced this pull request Nov 22, 2023
@hi-ogawa
fix: repro and veryfy remix-run/remix#8095
1a885e1
@hi-ogawa hi-ogawa mentioned this pull request Nov 22, 2023
Closed
hi-ogawa
hi-ogawa commented Nov 22, 2023
View reviewed changes
integration/vite-css-dev-express-test.ts
Comment on lines +55 to +67
// 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,
},
}
})
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.

@hi-ogawa hi-ogawa changed the title fix(remix-dev/vite): fix websocket double handleUpgrade error with custom vite server config fix(remix-dev/vite): fix websocket double handleUpgrade error with custom vite hmr server config Nov 22, 2023
@hi-ogawa hi-ogawa marked this pull request as ready for review November 22, 2023 03:55
@pcattori pcattori added the vite label Nov 22, 2023
@pcattori
Copy link
Contributor

pcattori commented Nov 22, 2023 •
edited
Loading

@hi-ogawa using hmr: false is definitely something we want to do but this PR seems also concerned with createViteServer API. So I made a separate PR with just the hmr: false changes #8108 since I want to consider how we handle server configuration and HTTPS interop separately. Need to dig into that myself more before deciding how to best test those features too

@hi-ogawa
Copy link
Contributor Author

@pcattori Thanks for taking a look! The blocker for users was child compiler initialized with a following type of vite config, so as long as it's gone, this issue #7953 should be resolved by your PR.

server: {
  hmr: {
    server: ...(user custom server)
  },
  middlewareMode: false,
}

@hi-ogawa hi-ogawa changed the title fix(remix-dev/vite): fix websocket double handleUpgrade error with custom vite hmr server config test(remix-dev/vite): test custom server.hmr.server vite config Nov 24, 2023
@Mordred Mordred mentioned this pull request Nov 29, 2023
Closed
1 task
@pcattori
Copy link
Contributor

Obsolete as we no longer ship APIs like unstable_createViteServer that wrap Vite's APIs.

@pcattori pcattori closed this Jan 12, 2024
@hi-ogawa hi-ogawa deleted the fix-vite-child-vite-server-double-handleUpgrade-custom-server branch January 13, 2024 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mordred Mordred Mordred left review comments

Assignees
No one assigned
Labels
CLA Signed vite
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hi-ogawa @pcattori @Mordred