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: needs es interop check for newly discovered deps #7243

Merged
merged 2 commits into from Mar 10, 2022

Conversation

patak-dev
Copy link
Member

@patak-dev patak-dev commented Mar 9, 2022

Description

This PR fixes the needs es interop check for newly discovered dependencies. It is an edge case, and this is why we didn't discover it so far. What needs to happen is that:

  • There is a missing dependency that needs es interop
  • and when re-processing the deps there is no need for a full-page reload.

And CI is going to fail for react-emotion because when we call moduleGraph.invalidateAll() before doing the full-reload after re-processing, there are requests that are on the fly and end up updating their module info... so when the browser requests them after the full reload... these modules are stale. @vursen this is what we were seeing in Vaadin with the 504 errors in the browser that didn't disappear even when reloading the page manually.

We need that a full-reload call will also stop every currently processing request to avoid modifying the module graph.

I think this could have been also present before, but now it is more visible because we aren't blocking the requests as before while processing. So a solution here may fix hard-to-replicate issues that we got in the past.

I'll work on a PR to fix the race condition next, I'm sending this PR to serve as a bug reproduction once CI fails.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@patak-dev
Copy link
Member Author

fa88544 failed as expected

db17172 fixes the race condition described in the description by avoiding caching the transform result of a module that was invalidated while processing

Once we approve this PR as a whole, I can create a PR for the race condition fix and then merge this one.

@patak-dev patak-dev requested a review from bluwy March 9, 2022 22:49
@patak-dev patak-dev added the p4-important Violate documented behavior or significantly improves performance (priority) label Mar 9, 2022
@patak-dev patak-dev added this to the 2.9 milestone Mar 9, 2022
@patak-dev
Copy link
Member Author

Here is a run of Vite Ecosystem CI against this PR:
https://github.com/vitejs/vite-ecosystem-ci/actions/runs/1961459259

All green (except an expected vite-plugin-ssr fail which is also present in main right now)

@patak-dev patak-dev merged commit ba3047d into main Mar 10, 2022
@patak-dev patak-dev deleted the fix/import-analysis-es-interop branch March 10, 2022 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p4-important Violate documented behavior or significantly improves performance (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants