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: store backwards compatible ssrModule and ssrError #18031

Merged
merged 6 commits into from
Sep 4, 2024

Conversation

sheremet-va
Copy link
Member

@sheremet-va sheremet-va commented Sep 4, 2024

Description

The previous ssrLoadModule compat layer only stored the entry point's ssrModule, this cases issues with astro.

This PR makes it so that every time the module is requested, we store the ssrModule value on the server's moduleGraph.

Copy link

stackblitz bot commented Sep 4, 2024

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

@sheremet-va
Copy link
Member Author

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 70f25e2: Open

suite result latest scheduled
histoire failure success
remix failure success
sveltekit success failure
vike failure success
vitest failure success

analogjs, astro, ladle, laravel, marko, nuxt, previewjs, quasar, qwik, rakkas, redwoodjs, storybook, unocss, vite-environment-examples, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-swc, vite-plugin-svelte, vite-plugin-vue, vite-setup-catalogue, vitepress, vuepress

@sheremet-va sheremet-va marked this pull request as ready for review September 4, 2024 12:41
@@ -137,6 +137,7 @@ export async function fetchModule(
return {
code: result.code,
file: mod.file,
serverId: mod.id!,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome! So good to see Astro working 🎉
Could this be called id directly? Is the id in the runner different than the one in the server?

Copy link
Member Author

@sheremet-va sheremet-va Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, they are different. Not intentionally, really. An artifact from vite-node that I am too afraid to change 😄

The ID in the module runner is the URL on the server module graph. The module runner caches by urls so it's easier to keep track of, it doesn't have multiple cache maps. It's also more similar to how browser caches urls, I guess.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let's merge this. Ok, if this is part of the public API for ModuleRunner. I'm thinking that we could maybe use url then everywhere we have id in the runner? 🤔
And then serverId can be just id.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think so

@patak-dev patak-dev merged commit cf8ced5 into vitejs:main Sep 4, 2024
12 checks passed
@sheremet-va sheremet-va deleted the fix/save-ssrmodule-ssrerror branch September 5, 2024 08:52
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.

2 participants