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

defjosiah · 2023-07-04T23:15:33Z

Description

With the cloudflare-workers template (and likely pages), there is a race condition. Sometimes the router that is used in
the import.meta setup in the RemixBrowser component file is undefined.

My guess is that the hmr update is sent before the component is hydrated (which is where the file global
let router: Router variable is assigned).

This adds a guard to the import.meta.hot.accept function, which waits for the router to be initialized before it is used in
the rest of the function.

Testing Strategy

I discovered this in devtools, I have a reproducing example locally (same as the issue this closes).

Without my change, you'll see that the router is undefined, and the error:

browser.js:42 Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'state')
    at browser.js:42:39
    at Object.emit (hmr-runtime:remix:hmr:64:9)
    at ws.onmessage (login:62:82)

is shown. There is also no HMR that works.

With my change, built and applied locally, it properly does HMR and has no errors:

I looked for places to add tests for this, but I don't see any component level tests for the component, and it looked like a large can of worms to attempt to add the first tests for this component. The hmr integration test will catch if this is an issue with the existing hmr setup.

Reproduction Steps

Start with the cloudflare-workers remix template
Try HMR, it may or may not work, if it doesn't work, it'll show that error (or Uncaught (in promise) TypeError: Failed to fetch dynamically imported module: http://localhost:8787/build/entry.client-EGKEKBAB.js)

changeset-bot · 2023-07-04T23:15:36Z

🦋 Changeset detected

Latest commit: fe8175d

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

This PR includes changesets to release 18 packages

Name	Type
@remix-run/react	Patch
@remix-run/testing	Patch
create-remix	Patch
remix	Patch
@remix-run/architect	Patch
@remix-run/cloudflare	Patch
@remix-run/cloudflare-pages	Patch
@remix-run/cloudflare-workers	Patch
@remix-run/css-bundle	Patch
@remix-run/deno	Patch
@remix-run/dev	Patch
@remix-run/eslint-config	Patch
@remix-run/express	Patch
@remix-run/netlify	Patch
@remix-run/node	Patch
@remix-run/serve	Patch
@remix-run/server-runtime	Patch
@remix-run/vercel	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

remix-cla-bot · 2023-07-04T23:15:36Z

Hi @defjosiah,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run.

Thanks!

- The Remix team

remix-cla-bot · 2023-07-04T23:18:24Z

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

defjosiah · 2023-07-04T23:25:58Z

I'm also running with --no-restart, so there's a chance it also fixes:

#6748

mauricevancooten · 2023-07-05T03:08:53Z

I'm using the cloudflare-workers template and can confirm I'm getting this error too. @defjosiah I'm looking forward to your fix!

jacob-ebey · 2023-07-05T19:26:56Z

@defjosiah I have a commit over here that I'd like to see added as part of this fix: #6774

defjosiah · 2023-07-05T19:31:03Z

@jacob-ebey: sounds good!
I can pull that in here (You can also feel free to push to my branch, but maybe the fork confuses that).

How do you want me to handle that?
I can also just accept your PR

jacob-ebey · 2023-07-05T19:35:19Z

@jacob-ebey: sounds good! I can pull that in here (You can also feel free to push to my branch, but maybe the fork confuses that).

How do you want me to handle that? I can also just accept your PR

Feel free to pull it into your fork / this PR if you'd like to do that!

…xtra pending HMR work

defjosiah · 2023-07-05T19:46:28Z

@jacob-ebey: done! Thanks for the extra error handling

defjosiah · 2023-07-05T20:12:10Z

Do I have to do anything else here, or will y'all handle merging it, etc.?

jacob-ebey · 2023-07-05T20:29:21Z

Do I have to do anything else here, or will y'all handle merging it, etc.?

We'll take care of it. Thanks for the contribution! 🎉

pcattori · 2023-07-05T21:29:57Z

packages/remix-react/browser.tsx

@@ -180,7 +202,7 @@ export function RemixBrowser(_props: RemixBrowserProps): ReactElement {
          window.__remixContext.future.v2_normalizeFormMethod,
      },
    });
-
+    


@jacob-ebey this looks like unintentional whitespace

pcattori · 2023-07-05T21:34:04Z

packages/remix-react/browser.tsx

+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;
+});


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)

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.

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.

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

defjosiah · 2023-07-07T16:47:25Z

Hi, should I be keeping this up to date?
Wasn't sure what the merging timeline is for a contribution.

pcattori · 2023-07-07T17:52:46Z

@defjosiah I'll review this with @jacob-ebey on Monday

github-actions · 2023-07-12T07:08:23Z

🤖 Hello there,

We just published version v0.0.0-nightly-c8936ac-20230712 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

github-actions · 2023-07-13T16:29:36Z

🤖 Hello there,

We just published version 1.19.0-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

github-actions · 2023-07-19T18:03:26Z

🤖 Hello there,

We just published version 1.19.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Guard usage of router with a promise for hmr

70df423

defjosiah mentioned this pull request Jul 4, 2023

HMR not working in cloudflare-pages template #6415

Closed

1 task

Sign CLA

005b186

remix-cla-bot bot added the CLA Signed label Jul 4, 2023

Add changeset

aba7ca6

pcattori self-assigned this Jul 5, 2023

defjosiah force-pushed the defjosiah/act/hmr-promise-router branch from aba7ca6 to 441848b Compare July 5, 2023 17:13

sergiodxa approved these changes Jul 5, 2023

View reviewed changes

pcattori assigned jacob-ebey and unassigned pcattori Jul 5, 2023

jacob-ebey mentioned this pull request Jul 5, 2023

Wait for router #6774

Closed

- handle promise rejection - notify after reload check to avoid any e…

59552f4

…xtra pending HMR work

defjosiah force-pushed the defjosiah/act/hmr-promise-router branch from 441848b to 59552f4 Compare July 5, 2023 19:44

jacob-ebey approved these changes Jul 5, 2023

View reviewed changes

Merge branch 'dev' into defjosiah/act/hmr-promise-router

6076545

pcattori reviewed Jul 5, 2023

View reviewed changes

brophdawg11 added the package:dev label Jul 10, 2023

brophdawg11 added the feat:dx Issues related to the developer experience label Jul 10, 2023

Merge branch 'dev' into defjosiah/act/hmr-promise-router

fe8175d

pcattori merged commit afb5413 into remix-run:dev Jul 11, 2023

github-actions bot mentioned this pull request Jul 13, 2023

chore: Update version for release (pre) #6828

Merged

defjosiah deleted the defjosiah/act/hmr-promise-router branch July 14, 2023 00:45

github-actions bot mentioned this pull request Jul 19, 2023

chore: Update version for release #6882

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

defjosiah commented Jul 4, 2023

changeset-bot bot commented Jul 4, 2023 •

edited

Loading

remix-cla-bot bot commented Jul 4, 2023

remix-cla-bot bot commented Jul 4, 2023

defjosiah commented Jul 4, 2023

mauricevancooten commented Jul 5, 2023

jacob-ebey commented Jul 5, 2023

defjosiah commented Jul 5, 2023

jacob-ebey commented Jul 5, 2023

defjosiah commented Jul 5, 2023

defjosiah commented Jul 5, 2023

jacob-ebey commented Jul 5, 2023 •

edited

Loading

pcattori Jul 5, 2023

pcattori Jul 5, 2023

defjosiah Jul 5, 2023

pcattori Jul 5, 2023 •

edited

Loading

defjosiah Jul 5, 2023

defjosiah commented Jul 7, 2023

pcattori commented Jul 7, 2023

github-actions bot commented Jul 12, 2023

github-actions bot commented Jul 13, 2023

github-actions bot commented Jul 19, 2023

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

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

Conversation

defjosiah commented Jul 4, 2023

Description

Testing Strategy

Reproduction Steps

changeset-bot bot commented Jul 4, 2023 • edited Loading

🦋 Changeset detected

remix-cla-bot bot commented Jul 4, 2023

remix-cla-bot bot commented Jul 4, 2023

defjosiah commented Jul 4, 2023

mauricevancooten commented Jul 5, 2023

jacob-ebey commented Jul 5, 2023

defjosiah commented Jul 5, 2023

jacob-ebey commented Jul 5, 2023

defjosiah commented Jul 5, 2023

defjosiah commented Jul 5, 2023

jacob-ebey commented Jul 5, 2023 • edited Loading

pcattori Jul 5, 2023

Choose a reason for hiding this comment

pcattori Jul 5, 2023

Choose a reason for hiding this comment

defjosiah Jul 5, 2023

Choose a reason for hiding this comment

pcattori Jul 5, 2023 • edited Loading

Choose a reason for hiding this comment

defjosiah Jul 5, 2023

Choose a reason for hiding this comment

defjosiah commented Jul 7, 2023

pcattori commented Jul 7, 2023

github-actions bot commented Jul 12, 2023

github-actions bot commented Jul 13, 2023

github-actions bot commented Jul 19, 2023

changeset-bot bot commented Jul 4, 2023 •

edited

Loading

jacob-ebey commented Jul 5, 2023 •

edited

Loading

pcattori Jul 5, 2023 •

edited

Loading