Fix HMR in MDX deps in Content Collections #9956
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 2709497 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
|
!preview cc-hmr |
|
Snapshots have been released for the following packages:
Publish LogBuild Log |
1 task
|
This fixes the issue for me. Copying the comment I made in the mentioned issue:
|
|
Just tested on the few Starlight related scenarios I had in my notes having that issue, and just like Hippo, it now works perfectly with the preview release 👍 |
Co-authored-by: Florian Lefebvre <[email protected]>
natemoo-re
approved these changes
Feb 4, 2024
| const collectedStyles = ${stringifiedStyles}; | ||
| const collectedScripts = ${stringifiedScripts}; | ||
|
|
||
| function getPropagatedAssets() { |
There was a problem hiding this comment.
Dang. Clever solution! Not too clever of course, just the right amount of clever. 👌
Princesseuh
approved these changes
Feb 4, 2024
| @@ -0,0 +1,29 @@ | |||
|
|
|||
| // TODO | |||
There was a problem hiding this comment.
Was there something to be done here?
| @@ -62,6 +62,9 @@ class VirtualVolume extends Volume { | |||
| class VirtualVolumeWithFallback extends VirtualVolume { | |||
| // Fallback to the real fs | |||
| readFile(p, ...args) { | |||
| if(p?.toString().includes('astro:dev-module-loader')) { | |||
| throw new Error('This module can\'t be loaded threw here. blah'); | |||
There was a problem hiding this comment.
Not sure how likely that error is, but if it's real, a good error message would be better so we don't eventually have to explain to a new contributor why that error happens, ha
bluwy
previously requested changes
Feb 5, 2024
There was a problem hiding this comment.
It seems like this could break markdoc?
astro/packages/integrations/markdoc/components/TreeNode.ts
Lines 126 to 128 in 6d273be
I'm not very comfortable with the .ssrModule assignment because it means crossing an explicitly separate boundary between the "Vite-plugin runtime" and "SSR app runtime". It can also make supporting other envs in the future, like workerd in dev, harder.
(Collapsing my comments because it's outdated)
It looks like the core issue here is that if /my/component/Foo.astro is invalidated, we should also invalidate /my/component/Foo.astro?astroPropagatedAssets hardly. I can't think of a cleaner fix today, but if we need to workaround it, maybe we should directly fix the invalidation, e.g. patching server.moduleGraph.invalidateModule, check if we're invalidating an .astro module, then try to invalidate the .astro?astroPropagatedAssets module as well?
I think what Vite can fix, which I can try to implement, is that the /my/component/Foo.astro?astroPropagatedAssets transform can call this.addWatchFile('/my/component/Foo.astro'), which implies that even if we import it in code, whenever the watched file is changed, we should still hard-invalidate it.
EDIT: I got a better idea of the problem now today. The issue is that when any imported modules recursively down from the propagated module is updated, the propagated module also needs to be invalidated. Because getStylesForURL and getScriptsForURL crawls all of its imported modules recursively to retrieve that.
The proper way is to register all of its imported modules with this.addWatchFile, maybe get...ForURL can return that based on the walked mod.file. Another workaround is to simply invalidate all propagated modules whenever handleHotUpdate is invoked, so that they are re-transformed on the next request. Or another workaround is this PR, so I could see this PR being something worth considering too 🤔 I think I'd still lean on the first way (in this paragraph) though.
|
!preview cc-hmr |
|
Snapshots have been released for the following packages:
Publish LogBuild Log |
|
lgtm, willing to try this approach. |
bluwy
approved these changes
Feb 6, 2024
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
entry.render()is called in dev, it gets fresh set of styles/scripts, so it should always be right.Testing
Docs
N/A, bug fix