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 useMatches returning different loader data on sub navigation on client side #5556

Closed
wants to merge 17 commits into from

Conversation

wizardlyhel
Copy link
Contributor

Closes: #

  • Docs
  • Tests

Problem: useMatches returning different route match data on sub navigation. This problem isn't obvious because the last useMatches hook call always returns the correct route's loader data

https://stackblitz.com/edit/remix-run-remix-askr4f?file=app/root.tsx

Navigate between home, test and test 2 page.

  1. On full page reload, you will see multiple logs from useMatches of the same route (This is fine and it is due to React re-renders)
  2. On sub navigation, you will see multiple logs from useMatches but of different route loader data. It is always the current route and the previous route

Notice that the server side console only logs loader data for the correct route

@changeset-bot
Copy link

changeset-bot bot commented Feb 23, 2023

🦋 Changeset detected

Latest commit: 5be576f

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

This PR includes changesets to release 18 packages
Name Type
remix Patch
@remix-run/react Patch
@remix-run/testing Patch
create-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
Copy link
Contributor

remix-cla-bot bot commented Feb 23, 2023

Hi @wizardlyhel,

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
Copy link
Contributor

remix-cla-bot bot commented Feb 23, 2023

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

@wizardlyhel wizardlyhel changed the title Fix useMatches causing client side render twice on sub navigation Fix useMatches causing client side returning different loader data on sub navigation Feb 23, 2023
@wizardlyhel wizardlyhel changed the title Fix useMatches causing client side returning different loader data on sub navigation Fix useMatches returning different loader data on sub navigation on client side Feb 23, 2023
@brophdawg11 brophdawg11 changed the base branch from main to dev February 28, 2023 16:29
@brophdawg11 brophdawg11 changed the base branch from dev to main February 28, 2023 16:29
@brophdawg11
Copy link
Contributor

brophdawg11 commented Feb 28, 2023

@wizardlyhel Thanks for this! Would you mind rebasing onto dev and re-pointing there since this contains a code change (docs-only changes can go straight to main)?

Once updated, I can bring over the new integration tests from #5601 as well and then we can get this merged

@wizardlyhel wizardlyhel changed the base branch from main to dev February 28, 2023 17:33
@wizardlyhel
Copy link
Contributor Author

wizardlyhel commented Feb 28, 2023

I mess up the rebase recreate the PR #5603 with dev

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.

8 participants