Skip to content

fix: resovedUrls is null after server restart#14890

Merged
patak-cat merged 3 commits intomainfrom
fix/resolved-urls-is-null-after-restart
Nov 7, 2023
Merged

fix: resovedUrls is null after server restart#14890
patak-cat merged 3 commits intomainfrom
fix/resolved-urls-is-null-after-restart

Conversation

@patak-cat
Copy link
Copy Markdown
Member

@patak-cat patak-cat commented Nov 6, 2023 •
edited
Loading

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

@bolt-new-by-stackblitz
Copy link
Copy Markdown

bolt-new-by-stackblitz bot commented Nov 6, 2023

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

@patak-cat patak-cat added the p3-minor-bug An edge case that only affects very specific usage (priority) label Nov 6, 2023
@patak-cat patak-cat mentioned this pull request Nov 6, 2023
Merged
9 tasks
bluwy
bluwy reviewed Nov 7, 2023
View reviewed changes
Copy link
Copy Markdown
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.

@patak-cat patak-cat added this to the 5.0 milestone Nov 7, 2023
@patak-cat patak-cat enabled auto-merge (squash) November 7, 2023 08:13
@patak-cat patak-cat merged commit bd4d29f into main Nov 7, 2023
@patak-cat patak-cat deleted the fix/resolved-urls-is-null-after-restart branch November 7, 2023 08:14
@andy0130tw
Copy link
Copy Markdown

andy0130tw commented Nov 18, 2023 •
edited
Loading

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.

@patak-cat patak-cat mentioned this pull request Nov 27, 2023
Merged
4 tasks
@yuan116
Copy link
Copy Markdown

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

@XiSenao XiSenao mentioned this pull request Dec 28, 2023
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bluwy bluwy bluwy approved these changes

Assignees

No one assigned

Labels

p3-minor-bug An edge case that only affects very specific usage (priority)

Projects

None yet

Milestone

5.0

Development

Successfully merging this pull request may close these issues.

4 participants

@patak-cat @andy0130tw @yuan116 @bluwy