feat: non-blocking needs interop

[patak-dev]

Description

Continuation of the work started by

• feat: non-blocking pre bundling of dependencies #6758
• feat: non-blocking scanning of dependencies #7379

We allowed requests to flow through the pipeline in #6758 while pre-bundling, but we needed to block them when:

1. the optimized dep src code was loaded (this is unavoidable)
2. in the importAnalysis plugin when importing an optimized dep, we needed to wait until the dependencies are pre-bundled to know if they require interop

Ideally, we only would like to block on 1. Awaiting in 2 blocks requests for any user source code that imports a dep. So in a Vue app, we will be blocking most requests because they all import 'vue'.

Proposal

This PR proposes a way out potentially speeding up cold start even further, with the tradeoff that some legacy packages that advertise themselves as ESM but include mixed CJS may now need to be manually added in the user config. I believe that we could push this "breaking change" in Vite 3.0, and keep pushing for the ecosystem to fix packages as we already do in many other cases in Vite.

Edit: with the latest commits, there is no longer a breaking change. Mixed ESM and CJS would still work, and the only diff would be that cold start would be a bit slower because of an extra page reload if they aren't added to optimizeDeps.needsInterop.

Details

The old needsInterop checks:
a. KNOWN_INTEROP_IDS -> true (only 'moment' is included right now, that was the lib that originated the complex mixed ESM and CJS checks)
b. there aren't exports or imports -> true (assumes it is CJS or UMD)
c. mixed ESM and CJS by checking exports mismatch after esbuild bundled the dep

Because of c., this needs to be computed after pre-bundling

The new needsInterop after this PR checks only a. and b., and a new list in optimizedDeps.needsInterop. So we can compute it before pre-bundling.

We still check c. after pre-bundling, but now to be able to warn the user of the package issue and hint that this can be solved by adding the dep id to optimizedDeps.needsInterop

If we would like to proceed with this change, we would need to check how many dependencies have mixed ESM and CJS. And how problematic it is for users to manually add them to the config. I like the idea of having some friction so some of these packages get PRs to fix them.

As an alternative, we could avoid the optimizedDeps.needsInterop list and instead add a optimizedDeps.acceptMixedSyntax (better name?) that defaults to false. If the user sets it to true, then we revert to the current approach. I think that optimizedDeps.needsInterop is better because users don't need to revert to the slower scheme just because of one faulty package, and because the implementation (the one in this PR) is simpler than what we would need to support acceptMixedSyntax.

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

[bluwy]

Awesome ⚡

[patak-dev]

Adding this one to the 3.0 milestone, as it would be good to test it a bit during the beta window. I will add docs in the meantime and we would need to discuss the addition of the new optimizeDeps.needsInterop option. This PR could now be merged even without this option (but I think it is a good idea to add it)

[bluwy]

The merged changes lgtm. Maybe we should add docs for optimizeDeps.needsInterop here too?

[patak-dev]

After the latest changes in the repo, this PR makes this error happens more often: https://github.com/vitejs/vite/runs/6407272224?check_suite_focus=true#step:11:130, I think it may be a race condition that we are uncovering. I'm very confused by this test though, I tried to fix the package json of the dep it is importing but then the error is consistent:

• test: missing type module in ssr-vue package #8147

@bluwy this dep is written in ESM + a .vue file, and it doesn't have "type": "module". I think adding it makes sense, but also it is strange that this works at all. @bluwy wouldn't this need your optimizeDeps extensions scheme? Maybe you better understand what the test is trying to do here.

[patak-dev]

@bluwy would you help me here? I think this test is not right and should be removed. It was added by:

• test: SSR externals on Windows #4980 #4986

The idea was to test an issue with the external detection heuristics, but added a Package with an uncompiled .vue file (that IIUC, it isn't what the Vue ecosystem does). We have been adding noExternal and other flags to try to make it pass but I think we should delete it. In that case, this PR would work. I'm having issues understanding why it works with this PR, maybe this shouldn't have worked before? Would this work if it was a .svelte file without your extensions feat?

[bluwy]

Pushed a commit to update ssr-vue tests. It seems like when navigating to /store on the first test, vuex is detected and Vite dynamically optimize that, and somehow it keeps causing fails on the forth test ('/'). Adding a networkidle wait on the first test fixes it. So I don't think we need to remove the external test, and I think it's good that we keep it as it's a usecase in the wild.

I also found out that playgrounds with custom serve.js needs it's tests to call page.goto manually. This was not the case in the ssr-vue tests, which when the first test fails, it causes fails for the following ones. This is now fixed (along for ssr-react). Otherwise, all tests shares the same page (pollution).

Hopefully the tests passes now.

[patak-dev]

Oh, awesome @bluwy! I still think this may not be the best test to have, but even better if we dont need to remove it because of this PR. Would you send these changes in a separate PR so we can merge them without waiting approval for this PR?

[bluwy]

Oh are there more stuff to work on for this PR? Thought it was in a ready state except for the tests. If so yeah I can create a separate PR.

[patak-dev]

We didnt discuss it in a meeting, and there is a new option 👀