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.

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: reload page when routeModules doesn't contain module for current route #6409

Conversation

n8agrin
Copy link
Contributor

@n8agrin n8agrin commented May 16, 2023

This can happen when entry.client.ts isn't fully loaded before a route change occurs.

The fix is to validate the route's module is loaded, and if not, simply reload the page. Credit to @brophdawg11 who suggested this approach here: #1757 (comment)

See #1757 for more info.

Closes: #1757

  • Docs
  • Tests

Testing Strategy:

  • added a new test (included in this PR) named browser-entry-test.ts

… route

This can happen when entry.client.ts isn't fully loaded before a route change occurs.

See remix-run#1757 for more info.
@changeset-bot
Copy link

changeset-bot bot commented May 16, 2023

🦋 Changeset detected

Latest commit: 8c18d95

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

@n8agrin
Copy link
Contributor Author

n8agrin commented May 16, 2023

@brophdawg11 This is the PR I referenced here #5166 (comment)

@brophdawg11 brophdawg11 self-assigned this May 22, 2023
n8agrin and others added 2 commits May 22, 2023 09:13
Co-authored-by: Leo Singer <leo.p.singer@nasa.gov>
@brophdawg11
Copy link
Contributor

Thanks for the PR @n8agrin - I pulled it down and did some testing - nice work getting an E2E test for this!

I made some minor alterations - let me know what you think:

  • I moved the check up into the initialization block of RemixBrowser since we don't need it to run on all re-renders, just on the initial hydration render.
  • I also changed the approach do to a simple URL comparison. The matches comparison worried me a bit since I think it could potentially put us into a loop if a match for the initial URL was invalid (somehow got deleted from a CDN maybe). If it had no chance to pass on the subsequent reload, we'd get into a reload loop I think?
    • Instead, now we're just SSR-ing the initial URL and comparing that against window.location. Then if we find a mismatch and reload, that should always break the loop.
  • I also removed the return <div/> since I don't know that it was much better of a UX. In my testing I was still seeing a flash of the error - and I think that's fine in these small edge cases.

@lpsinger
Copy link
Contributor

I noticed I was tagged to review this, but I'm not a Remix maintainer. Was this a mistake?

@brophdawg11
Copy link
Contributor

@lpsinger I think that's only because of your comment above from last month

Copy link
Contributor Author

@n8agrin n8agrin left a comment

Choose a reason for hiding this comment

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

🙏 this lgtm! Thanks for taking a pass @brophdawg11

@brophdawg11 brophdawg11 merged commit 3496f04 into remix-run:dev Jun 14, 2023
8 of 9 checks passed
@brophdawg11 brophdawg11 removed their assignment Jun 14, 2023
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-ad9adee-20230615 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
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-12440f3-20230616 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
Copy link
Contributor

🤖 Hello there,

We just published version 1.18.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
Copy link
Contributor

🤖 Hello there,

We just published version 1.18.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!

@yarbsemaj
Copy link

Hello @n8agrin, this PR is causing issue with CDNs. Our CDN is caching page html with query params on from tracking links (e.g.) Google. This is then been compared to the URL another user is visiting and viewing the cached versions of the page, when the mismatch occurs the page gets into an infinite reload loop. I can see this affecting many users who also use CDN's like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants