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: avoid outdated module to crash in importAnalysis after restart #13231

Merged
merged 8 commits into from May 17, 2023

Conversation

patak-dev
Copy link
Member

Description

wip potential fix for #13030


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@stackblitz
Copy link

stackblitz bot commented May 17, 2023

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

@patak-dev
Copy link
Member Author

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented May 17, 2023

📝 Ran ecosystem CI: Open

suite result
astro ❌ failure
histoire ✅ success
iles ✅ success
ladle ✅ success
laravel ✅ success
marko ✅ success
nuxt ✅ success
nx ✅ success
previewjs ✅ success
qwik ✅ success
rakkas ✅ success
sveltekit ❌ failure
unocss ✅ success
vite-plugin-ssr ❌ failure
vite-plugin-react ✅ success
vite-plugin-react-pages ✅ success
vite-plugin-react-swc ✅ success
vite-plugin-svelte ❌ failure
vite-plugin-vue ✅ success
vite-setup-catalogue ✅ success
vitepress ✅ success
vitest ✅ success

@patak-dev patak-dev changed the title fix: ensure module in graph before pretransfom in indexHtml middleware fix: avoid outdated module to crash in importAnalysis after restart May 17, 2023
@@ -73,7 +73,7 @@ export function transformRequest(
const pending = server._pendingRequests.get(cacheKey)
if (pending) {
return server.moduleGraph
.getModuleByUrl(removeTimestampQuery(url), options.ssr)
Copy link
Member Author

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

Copy link
Member

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 🙂

Comment on lines -257 to 262
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)
Copy link
Member Author

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.

@patak-dev
Copy link
Member Author

The issue is that server.moduleGraph is a completely new graph after the server restart
This may be an issue in other code paths too, maybe we should cache the moduleGraph as a local variable to each plugin, and not only cache server as a local variable.

This line makes server.moduleGraph completely unstable if we are using it in async contexts (so almost everywhere):

Object.assign(server, newServer)

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 moduleGraph but it isn't accounting for restarts

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.

Thanks for the fix!

@patak-dev patak-dev merged commit 3609e79 into main May 17, 2023
20 checks passed
@patak-dev patak-dev deleted the fix/pretransform-in-index-html-middleware branch May 17, 2023 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants