fix: resolvedUrls is null in plugin's configureServer after server restart by XiSenao · Pull Request #15450 · vitejs/vite

XiSenao · 2023-12-28T10:58:19Z

Description

Additional context

#14890 PR fixed the issue of indirectly referencing the dev server instance (e.g. referencing it In the delayed execution function), which was not effective when directly referencing the dev server instance after restarting the server (e.g. partial middlewares and configureServer hook in the plugin). vite will expose the dev server instance to the configureServer hook in the plugin, which also means that the dev server instance may be used by other hooks in the plugin. Therefore, I think it is necessary to maintain consistency of the dev server instance after server restart. as for the middlewares, I don't think any additional processing is necessary at the moment, as it may result in performance loss and inconsistent behavior before and after configuration. This fix adds a layer of proxy to the dev server reference in the configureServer hook of the plugin.

What is the purpose of this pull request?

Bug fix
New Feature
Documentation update
Other

Before submitting the PR, please make sure you do the following

Read the Contributing Guidelines, especially the Pull Request Guidelines.
Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
Update the corresponding documentation if needed.
Ideally, include relevant tests that fail without this PR but pass with it.

bolt-new-by-stackblitz · 2023-12-28T10:58:22Z

Run & review this pull request in StackBlitz Codeflow.

XiSenao · 2024-01-03T08:08:17Z

After restarting the server, does Vite not need to expose the new dev server instance to user plugins? @bluwy @sapphi-red

bluwy · 2024-01-08T05:59:35Z

Thanks for investigating this! The indirections in server handling today took me quite some time to understand 😄 So from what I understand, the configureServer hook is referencing the old server on init, so after _setInternalServer, it's still not referencing the new server, causing the problem.

The proxy seems like the only way best way to handle this. Though at this point, maybe it's easier to have createServer() simply return a Proxy, which proxies to whichever latest server internally. And that should simplify a lot of our old/new server handling. I think I'll let @patak-dev chime in on this before deciding the best way forward.

After restarting the server, does Vite not need to expose the new dev server instance to user plugins?

It should expose the new one. It's a bug that it doesn't since #14890

patak-cat

Thanks for the PR! Ya... this is getting trickier and trickier. I vote to merge this PR for the next minor and test it during the beta period we are just starting. And then evaluate @bluwy's idea of making createServer return a proxy for the next major. It would also be easier to explain and document how the server magical instance behaves.

XiSenao · 2024-01-08T17:25:11Z

The indirections in server handling today took me quite some time to understand 😄

oh, I am very sorry that my description has caused you trouble in understanding the meaning. In order to avoid further misunderstandings, I would like to provide a more detailed explanation of this issue.

During the server restart phase, a new dev Server will be created through the _createServer function.

vite/packages/vite/src/node/server/index.ts

Lines 988 to 992 in a621de8

    
           let newServer = null 
        
           try { 
        
             // delay ws server listen 
        
             newServer = await _createServer(inlineConfig, { ws: false }) 
        
           } catch (err: any) {

The configureServer hook will be executed within the _createServer function

vite/packages/vite/src/node/server/index.ts

Lines 716 to 718 in a621de8

    
           for (const hook of config.getSortedPluginHooks('configureServer')) { 
        
             postHooks.push(await hook(server)) 
        
           }

and at this point, the dev server instance passed to the configureServer hook can be considered as an new initialized dev server instance.

vite/packages/vite/src/node/server/index.ts

Line 433 in a621de8

let server: ViteDevServer = {

After creating a new dev server, existing dev server instances will be reused using the Object.assign method.

vite/packages/vite/src/node/server/index.ts

Lines 1002 to 1006 in a621de8

    
           // Assign new server props to existing server instance 
        
           const middlewares = server.middlewares 
        
           newServer._configServerPort = server._configServerPort 
        
           newServer._currentServerPort = server._currentServerPort 
        
           Object.assign(server, newServer)

Due to reusing the existing dev server instance, the subsequent updates to the properties are on the existing dev server instance rather than a new dev server instance.
In the #14890 PR, the _setInternalServer method is used to rebind the dev server instance inside the _createServer function, which is a good idea.

vite/packages/vite/src/node/server/index.ts

Lines 611 to 615 in a621de8

    
           _setInternalServer(_server: ViteDevServer) { 
        
             // Rebind internal the server variable so functions reference the user 
        
             // server instance after a restart 
        
             server = _server 
        
           },

However, the configureServer hook cannot access the re-bound dev Server instance in the _setInternalServer function due to closure reasons, leading to the problem in #15438.

vite/packages/vite/src/node/server/index.ts

Line 717 in a621de8

postHooks.push(await hook(server))

This is also what I mentioned in the description "which was not effective when directly referencing the dev server instance after restarting the server". Directly referencing the dev Server instance cannot access the server in the _setInternalServer function due to closure reasons, and that is the current problem.

The indirect reference to the dev server instance mentioned in the description refers to a situation similar to the following.

vite/packages/vite/src/node/server/index.ts

Lines 450 to 452 in a621de8

    
           transformRequest(url, options) { 
        
             return transformRequest(url, server, options) 
        
           },

When referencing the dev server instance in the function, the function will be triggered at some point in the future. At this point, the dev server instance instance obtained is the one re-bound in the _setInternalServer method, which is the correct behavior.

Though at this point, maybe it's easier to have createServer() simply return a Proxy, which proxies to whichever latest server internally.

It may be a lack of thinking on my part, I don't know why making createServer return a proxy can change the dev server reference in the configureServer hook.

bluwy · 2024-01-09T08:14:16Z

oh, I am very sorry that my description has caused you trouble in understanding the meaning. In order to avoid further misunderstandings, I would like to provide a more detailed explanation of this issue.

To be clear, I'm not criticising your code or explanations! 😅 I think they're good, but the code with have today (written by many people) made the whole flow hard to grasp, so that took me a while.

It may be a lack of thinking on my part, I don't know why making createServer return a proxy can change the dev server reference in the configureServer hook.

I mean that if createServer returns a proxy that only proxies API to the newer dev server. Then we can let _createServer be a lot simpler since it only needs to deal with its own let server: ViteDevServer = {... instance by itself. It doesn't need to know that it's server reference could mean a new dev server or anything. That's handled within createServer and restartServer only. Or in other words, we no longer need _setInternalServer and Object.assign. (Though this is just an idea)

sapphi-red

Thanks for the PR!
I also vote to merge this PR for 5.1. If we don't get any reports about the incompatibility using Proxy, then I think we can try @bluwy's idea with more confidence.

fix: resovedUrls is null after server restart

291c0a5

XiSenao force-pushed the fix/resovedUrls-is-null branch from f9e4cfe to 291c0a5 Compare December 28, 2023 12:48

XiSenao changed the title ~~fix: resolvedUrls is null in plugin's configureServer~~ fix: resolvedUrls is null in plugin's configureServer hook after server restart Dec 28, 2023

XiSenao changed the title ~~fix: resolvedUrls is null in plugin's configureServer hook after server restart~~ fix: resolvedUrls is null in plugin's configureServer after server restart Dec 28, 2023

chore: add comment

2990ddc

bluwy added the p3-minor-bug An edge case that only affects very specific usage (priority) label Jan 8, 2024

patak-cat approved these changes Jan 8, 2024

View reviewed changes

patak-cat added this to the 5.1 milestone Jan 8, 2024

sheremet-va mentioned this pull request Jan 9, 2024

fix(browser): fix browser testing url for https vitest-dev/vitest#4855

Merged

6 tasks

sapphi-red approved these changes Jan 10, 2024

View reviewed changes

patak-cat merged commit 653a48c into vitejs:main Jan 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: resolvedUrls is null in plugin's configureServer after server restart#15450

fix: resolvedUrls is null in plugin's configureServer after server restart#15450
patak-cat merged 2 commits intovitejs:mainfrom
XiSenao:fix/resovedUrls-is-null

XiSenao commented Dec 28, 2023 •

edited

Loading

Uh oh!

bolt-new-by-stackblitz bot commented Dec 28, 2023

Uh oh!

XiSenao commented Jan 3, 2024

Uh oh!

bluwy commented Jan 8, 2024 •

edited

Loading

Uh oh!

patak-cat left a comment

Uh oh!

XiSenao commented Jan 8, 2024 •

edited

Loading

Uh oh!

bluwy commented Jan 9, 2024

Uh oh!

sapphi-red left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

XiSenao commented Dec 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Additional context

What is the purpose of this pull request?

Before submitting the PR, please make sure you do the following

Uh oh!

bolt-new-by-stackblitz bot commented Dec 28, 2023

Uh oh!

XiSenao commented Jan 3, 2024

Uh oh!

bluwy commented Jan 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patak-cat left a comment

Choose a reason for hiding this comment

Uh oh!

XiSenao commented Jan 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bluwy commented Jan 9, 2024

Uh oh!

sapphi-red left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

XiSenao commented Dec 28, 2023 •

edited

Loading

bluwy commented Jan 8, 2024 •

edited

Loading

XiSenao commented Jan 8, 2024 •

edited

Loading