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

Improved HMR #3138

Merged
merged 3 commits into from
Apr 20, 2022
Merged

Improved HMR #3138

merged 3 commits into from
Apr 20, 2022

Conversation

natemoo-re
Copy link
Member

@natemoo-re natemoo-re commented Apr 18, 2022

Changes

  • Removes our fully-custom HMR and leans on the built-in vite:beforeUpdate event instead.
  • Fixes SSR-only HMR by tracking file dependencies in vite-plugin-astro
  • Fixes edge case with Astro/Svelte styles triggering a full reload because duplicate URLs were present and one was not being picked up as self-accepting.

Testing

Still looking into HMR tests

Docs

Bugfixes only

@changeset-bot
Copy link

changeset-bot bot commented Apr 18, 2022

🦋 Changeset detected

Latest commit: 4f993ae

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
astro Patch

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 github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Apr 18, 2022
@natemoo-re natemoo-re marked this pull request as ready for review April 19, 2022 18:18
const parser = new DOMParser();
import.meta.hot.on('astro:update', async ({ file }) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Custom HMR events are gone! Everything is handled by Vite now.

Comment on lines +33 to +35
if (hasAstroUpdate) {
return updatePage()
}
Copy link
Member Author

Choose a reason for hiding this comment

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

If an .astro file has been updated, we trigger our custom HMR here. This is the same event as before, just handled through Vite's built-in logic.

Comment on lines +85 to +87
// Bugfix: sometimes style URLs get normalized and end with `lang.css=`
// These will cause full reloads, so filter them out here
const mods = ctx.modules.filter(m => !m.url.endsWith('='));
Copy link
Member Author

Choose a reason for hiding this comment

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

This was an edge case that was causing full reloads when they weren't called for. Sometimes ?astro&type=astro&index=0&lang.css files would be duplicated in the module graph but represented as ?astro=&type=astro&index=0&lang.css= which would trigger a full reload. We're removing these from the hot update handler since they're always irrelevant.

// Bugfix: sometimes style URLs get normalized and end with `lang.css=`
// These will cause full reloads, so filter them out here
const mods = ctx.modules.filter(m => !m.url.endsWith('='));
const isSelfAccepting = mods.every(m => m.isSelfAccepting || m.url.endsWith('.svelte'));
Copy link
Member Author

Choose a reason for hiding this comment

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

Check all modules for isSelfAccepting, not just the file that triggered HMR. For some reason Svelte files aren't marked as self-accepting, but they are.

Comment on lines +177 to +190
// HACK: extract dependencies from metadata until compiler static extraction handles them
const metadata = transformResult.code.split('$$createMetadata(')[1].split('});\n')[0]
const pattern = /specifier:\s*'([^']*)'/g;
const deps = new Set();
let match;
while (match = pattern.exec(metadata)?.[1]) {
deps.add(match);
}
// // import.meta.hot.accept(["${id}", "${Array.from(deps).join('","')}"], (...mods) => mods);
// We need to be self-accepting, AND
// we need an explicity array of deps to track changes for SSR-only components
SUFFIX += `\nif (import.meta.hot) {
import.meta.hot.accept(mod => mod);
}`;
Copy link
Member Author

Choose a reason for hiding this comment

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

Rather than trigger HMR for only updates to this Astro file, we want to catch HMR updates for any dependencies. This is a bit hacky and could be cleaned up in the compiler, but it works for now.

Copy link
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

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

lgtm!

Copy link
Contributor

@tony-sull tony-sull left a comment

Choose a reason for hiding this comment

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

Nice! Looks good to me 👍

Great find on the full reloads in Svelte styles 🏆

@natemoo-re natemoo-re changed the title WIP: improved HMR Improved HMR Apr 19, 2022
@natemoo-re natemoo-re self-assigned this Apr 20, 2022
@natemoo-re natemoo-re merged commit 37a7a83 into main Apr 20, 2022
@natemoo-re natemoo-re deleted the fix/ssr-hmr branch April 20, 2022 21:46
@github-actions github-actions bot mentioned this pull request Apr 20, 2022
SiriousHunter pushed a commit to SiriousHunter/astro that referenced this pull request Feb 3, 2023
* WIP: improved HMR

* fix(hmr): improve hmr filtering to avoid full reloads

* chore: add changeset
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants