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.

Sign up for GitHub

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(dev): Wait for the router to be initialized in RemixBrowser before applying HMR changes #6767

Merged
merged 6 commits into from
Jul 11, 2023
Merged

fix(dev): Wait for the router to be initialized in RemixBrowser before applying HMR changes #6767

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
70df423
Guard usage of router with a promise for hmr
defjosiah Jul 4, 2023
005b186
Sign CLA
defjosiah Jul 4, 2023
aba7ca6
Add changeset
defjosiah Jul 4, 2023
59552f4
- handle promise rejection - notify after reload check to avoid any e…
jacob-ebey Jul 5, 2023
6076545
Merge branch 'dev' into defjosiah/act/hmr-promise-router
pcattori Jul 5, 2023
fe8175d
Merge branch 'dev' into defjosiah/act/hmr-promise-router
jacob-ebey Jul 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/mean-months-lick.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@remix-run/react": patch
---

fix router race condition for hmr
1 change: 1 addition & 0 deletions contributors.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -555,3 +555,4 @@
- mrkhosravian
- tanerijun
- ally1002
- defjosiah
29 changes: 28 additions & 1 deletion packages/remix-react/browser.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,19 @@ declare global {

let router: Router;
let hmrAbortController: AbortController | undefined;
let hmrRouterReadyResolve: ((router: Router) => void) | undefined;
// There's a race condition with HMR where the remix:manifest is signaled before
// the router is assigned in the RemixBrowser component. This promise gates the
// HMR handler until the router is ready
let hmrRouterReadyPromise = new Promise<Router>((resolve) => {
// body of a promise is executed immediately, so this can be resolved outside
// of the promise body
hmrRouterReadyResolve = resolve;
}).catch(() => {
// This is a noop catch handler to avoid unhandled promise rejection warnings
// in the console. The promise is never rejected.
return undefined;
});
Comment on lines +51 to +63
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use a Channel here instead so you don't have to manually set resolve and check for it later to make TS happy:

Suggested change
let hmrRouterReadyResolve: ((router: Router) => void) | undefined;
// There's a race condition with HMR where the remix:manifest is signaled before
// the router is assigned in the RemixBrowser component. This promise gates the
// HMR handler until the router is ready
let hmrRouterReadyPromise = new Promise<Router>((resolve) => {
// body of a promise is executed immediately, so this can be resolved outside
// of the promise body
hmrRouterReadyResolve = resolve;
}).catch(() => {
// This is a noop catch handler to avoid unhandled promise rejection warnings
// in the console. The promise is never rejected.
return undefined;
});
let routerReady = Channel.create<Router>()
// ...
let checkRouter = await routerReady.result()
if (!checkRouter.ok) {
console.error("...")
return
}
let router = checkRouter.value
// ...
routerReady.ok(router)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fine with me, however, it looks like that Channel implementation is in remix-dev package?

remix/packages/remix-dev/channel.ts

Line 28 in 0d22ee9

};

And it doesn't look like there's an import in this react package, so not sure how y'all would want to do that.

Also, typescript seems happy with it for me! I'm getting all the proper types out of this.

Copy link
Contributor

@pcattori pcattori Jul 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Oh yea good point. Not worth introducing the channel abstraction just for this if its not already in this package.

For TS, I just mean that if (hmrRouterReadyResolve) { is unnecessary with channel, but needed here since TS doesn't know that hmrRouterReadyResolve is guaranteed to be set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, got it!
Yeah, I really like that Channel abstraction, probably going to shamelessly use it somewhere else 😂


if (import.meta && import.meta.hot) {
import.meta.hot.accept(
Expand All @@ -59,6 +72,15 @@ if (import.meta && import.meta.hot) {
assetsManifest: EntryContext["manifest"];
needsRevalidation: Set<string>;
}) => {
let router = await hmrRouterReadyPromise;
// This should never happen, but just in case...
if (!router) {
console.error(
"Failed to accept HMR update because the router was not ready."
);
return;
}

let routeIds = [
...new Set(
router.state.matches
Expand Down Expand Up @@ -180,7 +202,7 @@ export function RemixBrowser(_props: RemixBrowserProps): ReactElement {
window.__remixContext.future.v2_normalizeFormMethod,
},
});

Copy link
Contributor

Choose a reason for hiding this comment

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

@jacob-ebey this looks like unintentional whitespace

// Hard reload if the path we tried to load is not the current path.
// This is usually the result of 2 rapid back/forward clicks from an
// external site into a Remix app, where we initially start the load for
Expand All @@ -197,6 +219,11 @@ export function RemixBrowser(_props: RemixBrowserProps): ReactElement {
console.error(errorMsg);
window.location.reload();
}

// Notify that the router is ready for HMR
if (hmrRouterReadyResolve) {
hmrRouterReadyResolve(router);
}
}

let [location, setLocation] = React.useState(router.state.location);
Expand Down