fix: keep track of ssr version of imported modules separately

[shYkiSto]

Description

Patch moduleGraph so it keeps separate lists of imported modules for browser (client) vs. server (SSR) version of a module. This fixes a scenario, where modules may use different sets of imports on browser vs. server (e.g., isomorphic modules, where Node.js (server) dependencies get stripped out from the browser version by running it trough some build-time transform). In which case Vite would fail to recognize and properly propagate the updates in server-only modules that always results in stale code running on the server (or, vice verse), because the module graph may get corrupted as a result of removing importers that are "no longer there", but in fact may still be imported in the counterpart version.

Additional context

---

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.
• Read the Pull Request Guidelines and follow the PR Title Convention.
• 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).
• Ideally, include relevant tests that fail without this PR but pass with it.

[rtsao]

If you've had a chance to review, is there any feedback or blockers that should be addressed before landing this? @patak-dev @bluwy @sapphi-red

[shYkiSto]

@patak-dev @bluwy @sapphi-red

Would be really nice to get your attention on this. Hopefully the issue description, and the test case provide enough context wrt the problem

[sapphi-red]

Thanks for the PR!
This looks good to me 👍
Would you resolve the conflict?

[shYkiSto]

@sapphi-red rebased 👍

[shYkiSto]

I noticed that changes introduced in #12599 render my test case invalid, so I had to "de-optimize" the browser module import by switching to dynamic import instead: https://github.com/vitejs/vite/compare/d11a3fa38cbf2372b42e23e48e055865c0205fbb..9821cb946a79b0d15fed3afd37468bf9a4380b0c

[sapphi-red]

Thank you

[patak-dev]

I also think we should merge this one. It may affect some projects though. We can do now an ecosystem-ci run, but there are a few projects red on main right now: https://github.com/vitejs/vite-ecosystem-ci/actions/runs/4571757840

[patak-dev]

/ecosystem-ci run

[vite-ecosystem-ci]

📝 Ran ecosystem CI: Open

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

[patak-dev]

Rakkas failing after this PR is new compared to main. @cyco130 would you check that splitting importedModules doesn't affect you?

[cyco130]

@patak-dev thanks for the heads up. I just pushed a fix, can you rerun the CI?

[patak-dev]

/ecosystem-ci run rakkas

[vite-ecosystem-ci]

📝 Ran ecosystem CI: Open

suite	result
rakkas	✅ success

[patak-dev]

Thanks @cyco130! Would you explain how this change affected you for refence?

[cyco130]

Thanks @cyco130! Would you explain how this change affected you for refence?

During dev, Vite normally loads an imported CSS file a bit late, causing FOUC until the JavaScript is evaluated. Rakkas scans the module graph to see what CSS files will be needed and injects the styles into the HTML in advance.

After this PR, module.importedModules comes empty on the first load because the module hasn't yet been compiled for the client, causing the FOUC test to fail.

So I switched to module.ssrImportedModules ?? module.importedModules (to remain compatible with previous versions). We know for sure that ssrImportedModules has been populated since the server version of the module was already loaded and evaluated to server-side render the page.

[bluwy]

Thanks for the explanation @cyco130. I realize many frameworks including SvelteKit and Astro does this too, and locally it has caused a failure for Astro for me. This seems a bit breaking than expected 🤔

We might still be able to write off as a bug fix, but we should probably ping the ecosystem first, or maybe find a workaround.

[cyco130]

Thanks for the explanation @cyco130. I realize many frameworks including SvelteKit and Astro does this too, and locally it has caused a failure for Astro for me. This seems a bit breaking than expected thinking

We might still be able to write off as a bug fix, but we should probably ping the ecosystem first, or maybe find a workaround.

Just an idea: Have all three: importedModules, browserImportedModules, and ssrImportedModules where importedModules is the union of both and mark it deprecated, to be removed on the next major.

[bluwy]

I think I'll fix this in Astro for now. It looks like SvelteKit and Nuxt are conscious that importedModules are for client only, and for SSR they're using node.ssrTransformResult.deps instead. I think we can go with this still.

[cyco130]

This discussion inspired this: #12678

[patak-dev]

I'm starting to lean towards waiting at least until the next minor to merge this PR. We should give some time to the ecosystem to react. It would also be good to have a bit more time to check the implications of this change. I think this is good given the current design, but it is starting to show that maybe mixing the two graphs under the same structure isn't the best long-term.

[patak-dev]

/ecosystem-ci run

[vite-ecosystem-ci]

📝 Ran ecosystem CI: Open

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

[patak-dev]

Hey @shYkiSto! Sorry it is taking a bit long to merge this one. We discussed the PR in a team meeting and decided we would like to avoid the breaking change. So what we're thinking is to add two new sets:

• ssrImportedModules
• clientImportedModules

And then keep the current importedModules as a getter that will merge the two sets on the fly. We should also avoid using importedModules internally and use ssrImportedModules or clientImportedModules directly.
Let us know if you'd like to modify the PR, if not we can do it. Thanks!

For reference, we also discussed the possibility of splitting the current module graph into clientModuleGraph and ssrModuleGraph. If we end up having to prefix each property with ssr and client in the nodes it could be cleaner to keep both graphs separate. This would be a bigger change for the ecosystem though so we better fix now the bug you found. But it is something to have in mind in the future.

[shYkiSto]

@patak-dev, sure thing! I'll try to get this updated sometime next week 👍

[shYkiSto]

@patak-dev, this should be ready for review. I've also noticed that the regression test I added has been somewhat flaky in CI, not sure if we should merge it as-is, or perhaps we'll be better off without it? Let me know in case you have any recommendation on how to make it more reliable

• 🔴 Failed on the 1st attempt: https://github.com/vitejs/vite/actions/runs/5095153252/jobs/9159788554#step:14:103
• 🔴 Failed on the 2nd attempt: https://github.com/vitejs/vite/actions/runs/5095153252/jobs/9217541946#step:14:103
• 🟢 Passed the 3rd time: https://github.com/vitejs/vite/actions/runs/5095153252/jobs/9255748483#step:14:31

Also, is this something you'd like to reconsider to include in the next minor release? I believe this change does not introduce any breaking changes anymore?

[patak-dev]

@shYkiSto, yes, let's merge this in Vite 4.4. You could add untilUpdated in the tests you see are causing issues. I'll run vite ecosystem CI once more now.

[patak-dev]

/ecosystem-ci run

[vite-ecosystem-ci]

📝 Ran ecosystem CI: Open

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