-
-
Notifications
You must be signed in to change notification settings - Fork 699
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: HMR #2316
base: main
Are you sure you want to change the base?
fix: HMR #2316
Conversation
☁️ Nx Cloud ReportCI is running/has finished running commands for commit d6ccf25. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 2 targetsSent with 💌 from NxCloud. |
@@ -142,7 +142,7 @@ export function compileCodeSplitReferenceRoute(opts: ParseAstOptions) { | |||
|
|||
programPath.pushContainer('body', [ | |||
template.statement( | |||
`function DummyComponent() { return null }`, | |||
`export function TSR_DummyComponent() { return null }`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This forces Fast Refresh to process the original route file even if we end up extracting the HMR passing conditions out of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marking the route as an HMR boundary without handling HMR will lead to stale updates. Instead using the patch provided by @schiller-manuel in discord is good and both mark the module as an self accepting without fast refresh code and handle the update.
} | ||
return null | ||
}, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look ma'! No resolveId
!
if ( | ||
fileIsInRoutesDirectory(id, userConfig.routesDirectory) || | ||
id.includes(splitPrefix) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I honestly don't know if this was still necessary or not....
componentId = splitNode.id?.name || componentId | ||
programPath.pushContainer( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is great. Now the component is registered to fast refresh. Ideally people should not write anonymous component, but in that case giving it the name SplitComponent
is better than having anonymous one
@@ -142,7 +142,7 @@ export function compileCodeSplitReferenceRoute(opts: ParseAstOptions) { | |||
|
|||
programPath.pushContainer('body', [ | |||
template.statement( | |||
`function DummyComponent() { return null }`, | |||
`export function TSR_DummyComponent() { return null }`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marking the route as an HMR boundary without handling HMR will lead to stale updates. Instead using the patch provided by @schiller-manuel in discord is good and both mark the module as an self accepting without fast refresh code and handle the update.
fd5f6d1
to
a5c79f6
Compare
Forces the root to be HMR ready Co-Authored-By: Kenta Iwasaki <[email protected]>
a5c79f6
to
8e7ae0c
Compare
@@ -20,7 +28,12 @@ export function getFullRouterManifest() { | |||
if (process.env.NODE_ENV === 'development') { | |||
rootRoute.assets.push({ | |||
tag: 'script', | |||
children: `window.__vite_plugin_react_preamble_installed__ = true`, | |||
attrs: { type: 'module' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ArnaudBarre what's your take on this?
Very good work! |
We've been working quite successful on this branch, what's missing to merge? |
Any timeline on when this might be merged? |
Is it possible to have a status update on this? Having to refresh the page for every change is a terrible DX 😢 |
No description provided.