-
Notifications
You must be signed in to change notification settings - Fork 57
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
Update refresh utils for Remix support #191
Conversation
src/refresh-runtime.js
Outdated
@@ -581,41 +581,60 @@ function debounce(fn, delay) { | |||
}; | |||
} | |||
|
|||
const enqueueUpdate = debounce(performReactRefresh, 16); | |||
const enqueueUpdate = debounce(() => { | |||
window.__beforePerformReactRefresh?.(); |
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 guess it'd nice to support registering multiple functions.
For example:
const hooks = []
window.__registerBeforePerformReactRefresh = (cb) => {
hooks.push(cb)
}
// run in enqueueUpdate
hooks.forEach(hook => hook())
// framework code
window.__registerBeforePerformReactRefresh(() => {
// do something that's needed
})
Another way to expose this functionality is to use an exported function:
import { registerBeforePerformReactRefreshHook } from '/@react-refresh-api'
registerBeforePerformReactRefreshHook(() => {
// do something that's needed for that framework
})
I'm not sure which is better.
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 made me remind that Remix need this to be async
The second option is cleaner, but requires the remix client to passe through Vite pipeline or to handle base path, and I'm not sure it's the case for now
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.
If this interface meets the needs of the remix, it looks good to me.
__beforePerformReactRefresh
&__getReactRefreshIgnoredExports
will not be part of semver until Remiix on Vite is stable (and without Fast Refresh related stuff being vendored).I'll do the same on the Babel plugin once I've got some positif feedback from @pcattori