-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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: avoid outdated module to crash in importAnalysis after restart #13231
Conversation
Run & review this pull request in StackBlitz Codeflow. |
/ecosystem-ci run |
📝 Ran ecosystem CI: Open
|
@@ -73,7 +73,7 @@ export function transformRequest( | |||
const pending = server._pendingRequests.get(cacheKey) | |||
if (pending) { | |||
return server.moduleGraph | |||
.getModuleByUrl(removeTimestampQuery(url), options.ssr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to the PR, the timestamp is already removed by getModuleByUrl
. We can move it to a separate PR if it is too noisy here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't mind mixing it here as it's only few line changes, so I'm fine either ways 🙂
if (!importerModule && depsOptimizer?.isOptimizedDepFile(importer)) { | ||
// Ids of optimized deps could be invalidated and removed from the graph | ||
// Return without transforming, this request is no longer valid, a full reload | ||
if (!importerModule) { | ||
// When the server is restarted, the module graph is cleared, so we | ||
// return without transforming. This request is no longer valid, a full reload | ||
// is going to request this id again. Throwing an outdated error so we | ||
// properly finish the request with a 504 sent to the browser. | ||
throwOutdatedRequest(importer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be the fix for #13030, the rest of the PR are extra guards I think we should also apply.
The issue is that This line makes vite/packages/vite/src/node/server/index.ts Line 851 in 0cbd818
I think we have code like this in some code paths: const module = await server.moduleGraph.ensureModuleByUrl(...)
...
await server.moduleGraph.getModuleByUrl(url) This would be fine if we always use the same |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix!
Description
wip potential fix for #13030
What is the purpose of this pull request?