fix: keep track of ssr version of imported modules separately #11973

shYkiSto · 2023-02-08T03:06:59Z

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 · 2023-03-07T18:36:16Z

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 · 2023-03-22T20:42:43Z

@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 · 2023-03-30T12:33:02Z

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

shYkiSto · 2023-03-30T17:16:10Z

@sapphi-red rebased 👍

shYkiSto · 2023-03-31T00:17:09Z

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 · 2023-03-31T07:11:23Z

/ecosystem-ci run

vite-ecosystem-ci · 2023-03-31T07:11:50Z

📝 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 · 2023-03-31T07:39:56Z

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

cyco130 · 2023-03-31T09:28:50Z

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

patak-dev · 2023-03-31T09:35:06Z

/ecosystem-ci run rakkas

vite-ecosystem-ci · 2023-03-31T09:35:30Z

📝 Ran ecosystem CI: Open

suite	result
rakkas	✅ success

patak-dev · 2023-03-31T09:49:26Z

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

cyco130 · 2023-03-31T10:00:37Z

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 · 2023-03-31T10:24:23Z

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 · 2023-03-31T11:06:40Z

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 · 2023-03-31T12:11:25Z

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 · 2023-03-31T12:49:37Z

This discussion inspired this: #12678

patak-dev · 2023-03-31T21:15:48Z

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 · 2023-04-19T08:59:39Z

/ecosystem-ci run

vite-ecosystem-ci · 2023-04-19T09:00:05Z

📝 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 · 2023-05-05T15:59:40Z

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 · 2023-05-11T22:32:11Z

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

shYkiSto · 2023-06-13T18:30:31Z

@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 · 2023-06-13T19:16:46Z

@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 · 2023-06-13T19:16:54Z

/ecosystem-ci run

vite-ecosystem-ci · 2023-06-13T19:17:18Z

📝 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

shYkiSto force-pushed the fix-split-ssr-imported-modules branch 3 times, most recently from 4044f11 to c299a3d Compare February 8, 2023 03:39

shYkiSto changed the title ~~fix: keep track of ssr imported modules separately~~ fix: keep track of ssr version of imported modules separately Feb 8, 2023

bluwy added the feat: ssr label Feb 16, 2023

sapphi-red added the p3-minor-bug An edge case that only affects very specific usage (priority) label Mar 30, 2023

shYkiSto force-pushed the fix-split-ssr-imported-modules branch 2 times, most recently from c7dfd63 to d11a3fa Compare March 30, 2023 16:55

shYkiSto force-pushed the fix-split-ssr-imported-modules branch from d11a3fa to 9821cb9 Compare March 31, 2023 00:14

sapphi-red previously approved these changes Mar 31, 2023

View reviewed changes

bluwy previously approved these changes Mar 31, 2023

View reviewed changes

patak-dev previously approved these changes Mar 31, 2023

View reviewed changes

bluwy mentioned this pull request Mar 31, 2023

Handle Vite 4.3 module graph changes withastro/astro#6716

Closed

patak-dev added this to the 5.0 milestone Apr 19, 2023

patak-dev marked this pull request as draft May 5, 2023 15:59

shYkiSto dismissed stale reviews from patak-dev, bluwy, and sapphi-red via ea14ebf May 26, 2023 21:20

shYkiSto force-pushed the fix-split-ssr-imported-modules branch from 724142a to ea14ebf Compare May 26, 2023 21:20

shYkiSto marked this pull request as ready for review May 26, 2023 21:25

shYkiSto force-pushed the fix-split-ssr-imported-modules branch from ea14ebf to a60bd7f Compare May 26, 2023 21:37

rtsao approved these changes Jun 5, 2023

View reviewed changes

shYkiSto requested a review from patak-dev June 13, 2023 18:45

shYkiSto force-pushed the fix-split-ssr-imported-modules branch from a60bd7f to 18e5808 Compare June 13, 2023 21:15

fix: keep track of ssr imported modules separately

6a1eac0

shYkiSto force-pushed the fix-split-ssr-imported-modules branch from 18e5808 to 6a1eac0 Compare June 13, 2023 21:33

bluwy approved these changes Jun 15, 2023

View reviewed changes

patak-dev merged commit 8fe6952 into vitejs:main Jun 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: keep track of ssr version of imported modules separately #11973

fix: keep track of ssr version of imported modules separately #11973

shYkiSto commented Feb 8, 2023 •

edited

Loading

rtsao commented Mar 7, 2023

shYkiSto commented Mar 22, 2023

sapphi-red commented Mar 30, 2023

shYkiSto commented Mar 30, 2023

shYkiSto commented Mar 31, 2023 •

edited

Loading

sapphi-red left a comment

patak-dev left a comment

patak-dev commented Mar 31, 2023

vite-ecosystem-ci bot commented Mar 31, 2023 •

edited

Loading

patak-dev commented Mar 31, 2023

cyco130 commented Mar 31, 2023

patak-dev commented Mar 31, 2023

vite-ecosystem-ci bot commented Mar 31, 2023 •

edited

Loading

patak-dev commented Mar 31, 2023

cyco130 commented Mar 31, 2023

bluwy commented Mar 31, 2023

cyco130 commented Mar 31, 2023 •

edited

Loading

bluwy commented Mar 31, 2023

cyco130 commented Mar 31, 2023

patak-dev commented Mar 31, 2023

patak-dev commented Apr 19, 2023

vite-ecosystem-ci bot commented Apr 19, 2023 •

edited

Loading

patak-dev commented May 5, 2023

shYkiSto commented May 11, 2023

shYkiSto commented Jun 13, 2023 •

edited

Loading

patak-dev commented Jun 13, 2023

patak-dev commented Jun 13, 2023

vite-ecosystem-ci bot commented Jun 13, 2023 •

edited

Loading

fix: keep track of ssr version of imported modules separately #11973

fix: keep track of ssr version of imported modules separately #11973

Conversation

shYkiSto commented Feb 8, 2023 • edited Loading

Description

Additional context

What is the purpose of this pull request?

Before submitting the PR, please make sure you do the following

rtsao commented Mar 7, 2023

shYkiSto commented Mar 22, 2023

sapphi-red commented Mar 30, 2023

shYkiSto commented Mar 30, 2023

shYkiSto commented Mar 31, 2023 • edited Loading

sapphi-red left a comment

Choose a reason for hiding this comment

patak-dev left a comment

Choose a reason for hiding this comment

patak-dev commented Mar 31, 2023

vite-ecosystem-ci bot commented Mar 31, 2023 • edited Loading

patak-dev commented Mar 31, 2023

cyco130 commented Mar 31, 2023

patak-dev commented Mar 31, 2023

vite-ecosystem-ci bot commented Mar 31, 2023 • edited Loading

patak-dev commented Mar 31, 2023

cyco130 commented Mar 31, 2023

bluwy commented Mar 31, 2023

cyco130 commented Mar 31, 2023 • edited Loading

bluwy commented Mar 31, 2023

cyco130 commented Mar 31, 2023

patak-dev commented Mar 31, 2023

patak-dev commented Apr 19, 2023

vite-ecosystem-ci bot commented Apr 19, 2023 • edited Loading

patak-dev commented May 5, 2023

shYkiSto commented May 11, 2023

shYkiSto commented Jun 13, 2023 • edited Loading

patak-dev commented Jun 13, 2023

patak-dev commented Jun 13, 2023

vite-ecosystem-ci bot commented Jun 13, 2023 • edited Loading

shYkiSto commented Feb 8, 2023 •

edited

Loading

shYkiSto commented Mar 31, 2023 •

edited

Loading

vite-ecosystem-ci bot commented Mar 31, 2023 •

edited

Loading

vite-ecosystem-ci bot commented Mar 31, 2023 •

edited

Loading

cyco130 commented Mar 31, 2023 •

edited

Loading

vite-ecosystem-ci bot commented Apr 19, 2023 •

edited

Loading

shYkiSto commented Jun 13, 2023 •

edited

Loading

vite-ecosystem-ci bot commented Jun 13, 2023 •

edited

Loading