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: resovedUrls is null after server restart #14890

Merged
merged 3 commits into from
Nov 7, 2023

Conversation

patak-dev
Copy link
Member

@patak-dev patak-dev commented Nov 6, 2023

Description

Fixes a regression introduced by:

To reproduce, start a dev server, change the config, and see that server URLs are re-printed even if they haven't changed. This was reported at #14418 (comment)

newServer.resolvedUrls was used here, but after the PR it was changed to server.resolvedUrls here.

The problem is that when restarting, we keep the prev server instance and replace its guts with the new server here

  // Assign new server props to existing server instance
  Object.assign(server, newServer)

The newServer functions still refer internally to the new instance. In the listen function, we do

server.resolvedUrls = ...

This happens after the Object.assign so it only gets set in newServer.

This PR fixes the issue with a new internal function to swap the internal server variable in the new server so functions start to refer to it.

An alternative could be to use this on functions. It will mean that server functions are no longer binded. I think this could be ok as we don't document this anywhere, but it is a big change.

This PR may fix other issues, for example, at close we were setting resolvedUrls to null, this never affected the proper server after a restart. There could be other similar cases for other server properties.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Copy link

stackblitz bot commented Nov 6, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@patak-dev patak-dev added the p3-minor-bug An edge case that only affects very specific usage (priority) label Nov 6, 2023
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Nice fix. One small nit below, but otherwise I think this is also a better fix for now instead of this. Seems like it'll only matter for properties that are re-assigned.

packages/vite/src/node/server/index.ts Outdated Show resolved Hide resolved
@patak-dev patak-dev added this to the 5.0 milestone Nov 7, 2023
@patak-dev patak-dev enabled auto-merge (squash) November 7, 2023 08:13
@patak-dev patak-dev merged commit bd4d29f into main Nov 7, 2023
15 checks passed
@patak-dev patak-dev deleted the fix/resolved-urls-is-null-after-restart branch November 7, 2023 08:14
@andy0130tw
Copy link

andy0130tw commented Nov 18, 2023

I found that this patch might be useful on fixing a buggy behavior long existed in Vite v4's dev server running in middleware mode, and hoped that this patch could be backported.

Suppose that I create a server in middleware mode with server = createServer(...). The server can then be restarted with await server.restartServer(false). In the function restartServer(server), a new server is created, and then there is a step Object.assign(server, newServer) to let the original server object function behave like the new one, and the new server object's reference is thrown away.

However there is a catch: All new methods have server bound to the new server, including the restart function. When the server is restarted the second time, the succeeding Object.assign copies methods of the third server to the second one, not the original one. As a result, the original server would stay stale forever. Since the reference to newer server is lost, I can think of no way to update the server other than to recreate a fresh one.

By making the server object swappable, which this patch implements and is merged in v5, I may be able to workaround the above-mentioned issue. Would you mind backporting this patch to v4? I can help on making a separate issue.

@yuan116
Copy link

yuan116 commented Dec 6, 2023

Hi, I am developing a plugin for my own use
and found out the resolvedUrls is still null in plugin's configureServer after server restarted, but the printrUrls working fine

Environment:
node: v21.2.0
npm: 9.5.0

this is the little hack that I use to retrieve after server listen
Screenshot 2023-12-06 at 17 29 00

before server restart
Screenshot 2023-12-06 at 17 35 39

after server restart
Screenshot 2023-12-06 at 17 35 54

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants