Simplify HMR handling #5811
Simplify HMR handling #5811
Conversation
🦋 Changeset detectedLatest commit: b005ea6 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
the
pkg: astro
| @@ -37,7 +35,7 @@ export async function compile({ | |||
| // use `sourcemap: "both"` so that sourcemap is included in the code | |||
| // result passed to esbuild, but also available in the catch handler. | |||
| transformResult = await transform(source, { | |||
| moduleId, | |||
| moduleId: filename, | |||
| pathname: filename, | |||
There was a problem hiding this comment.
I found the filename and the moduleId passed are always the same (absolute paths or virtual ids). They could only differ in the resolveId hook, but since we're compiling in the transform hook, it doesn't affect us.
In a future compiler PR, I'll merge moduleId and pathname options into one.
There was a problem hiding this comment.
So filename is the module ID right? To do head propagation we rely on the moduleId being passed to the compiler to match what is found in the module graph. Just want to make sure that is still going to be happening here.
There was a problem hiding this comment.
Yup, I tested it and saw it's always identical, even for virtual files. I think the unit test should cover if not as I was able to get it to fail when I pass the wrong id during manual testing.
There was a problem hiding this comment.
I did another quick review, and I think this part I guess I'm slightly unsure:
astro/packages/astro/src/vite-plugin-astro/index.ts
Lines 244 to 245 in 8404121
When you added the context.modules[0]?.id, is it because it differs from context.file? If I understand the module graph right, Vite's code should have them equal in most case.
There was a problem hiding this comment.
I think I did that out of "correctness" rather than a concern about context.file. So I think we can go with the change for now and see what happens.
| // note: don’t claim .astro files with resolveId() — it prevents Vite from transpiling the final JS (import.meta.glob, etc.) | ||
| async resolveId(id, from, opts) { |
There was a problem hiding this comment.
This entire resolveId hook is removed in favour of the next review comment.
| const normalPlugin: vite.Plugin = { | ||
| name: 'astro:build:normal', | ||
| resolveId(id) { | ||
| // If Vite resolver can't resolve the Astro request, it's likely a virtual Astro file, fallback here instead | ||
| const parsedId = parseAstroRequest(id); | ||
| if (parsedId.query.astro) { | ||
| return id; | ||
| } | ||
| }, | ||
| }; |
There was a problem hiding this comment.
... Continuing. This resolveId hook should cover all our cases instead.
The reason we can remove the pre plugin resolveId completely is because the paths can already be resolved by Vite's resolver plugin. Pre plugins run before Vite's resolver. Previously we try to resolve it ourselves, and the edge cases compound because we had to emulate the Vite resolver algorithm to get the flow right (which isn't feasible)
Now we let Vite handle instead, and only catch those that don't work with a normal plugin, which runs after Vite's resolver.
| @@ -196,20 +132,14 @@ export default function astro({ settings, logging }: AstroPluginOptions): vite.P | |||
| return; | |||
| } | |||
|
|
|||
| const filename = normalizeFilename(parsedId.filename, config); | |||
There was a problem hiding this comment.
In other diffs, you'll see that I had removed normalizeFilename. That's because normalizeFilename was previously used to emulate Vite's resolver algorithm, and only needed in a pre plugins resolveId.
Since this is a transform hook, we can remove it as it's already normalized by Vite.
| async load(id, opts) { | ||
| const parsedId = parseAstroRequest(id); | ||
| const query = parsedId.query; | ||
| if (!query.astro) { | ||
| return null; | ||
| } | ||
| // For CSS / hoisted scripts, the main Astro module should already be cached | ||
| const filename = normalizeFilename(parsedId.filename, config); |
There was a problem hiding this comment.
This still needs a normalizeFilename in load as it may receive a root-relative path, like /src/components/Foo.astro. This seems to only happen in tests (likely the mocked fs used?), but I leave it here for now just in case.
matthewp
force-pushed
the
try-simplify-hmr
branch
from
d396904
to
b005ea6
Compare
|
HMR (Hot Module Replacement) vs LR (Live Reload) - misused keyword? even on an empty project, there is no such thing as HMR - on each change page reloads only. So HMR does not work. Try changing |
Changes
Simplifies
vite-plugin-astro'sresolveIdhook. It looks like we were adding special cases there to get HMR working, but I found it possible to do less. I'll elaborate more in the review comments below.Testing
Existing tests that cover the HMR behaviour should pass. I checked the git history and made sure or edge case changes we have did have tests, so this should be safe.
Docs
n/a. internal change.