Allow navigation in child useEffects and fix intermittent issue where browser automation fails to navigate due to race conditions.

[42shadow42]

trafficstars

Fixes https://github.com/remix-run/react-router/issues/8809

[remix-cla-bot[bot]]

Hi @42shadow42,

Welcome, and thank you for contributing to React Router!

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 [email protected].

Thanks!

- The Remix team

[timdorr]

Can you please revert the changes to the compact imports? They are intentional. This is explained here: https://github.com/remix-run/react-router/blob/f3d87dcc91fbd6fd646064b88b4be52c15114603/packages/react-router-dom-v5-compat/index.ts#L1-L48

[42shadow42]

Sure I can! I just read that now and the explanation clarifies it a little but makes a claim that I don't understand fully. It claims that npm/node doesn't allow multiple versions of the same package, when I've done that before. There may be some complexity I don't understand, but if not I'd consider use aliases to for compatibility. I'll revert the updates when I get a chance.

[joshribakoff-sm]

@42shadow42 I think to sign the CLA you just need to add your username to https://github.com/42shadow42/react-router/blob/dev/contributors.yml

[changeset-bot[bot]]

⚠️ No Changeset found

Latest commit: 0f8205e7e14cdef01f79c738cade5325f49fb699

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[RShergold]

Is there any progress on this Pull Request? This will fix the bug that currently breaks all our tests meaning we can finally upgrade to React Router 6.

[github-actions[bot]]

This PR has been automatically marked stale because we haven't received a response from the original author in a while 🙈. This automation helps keep the issue tracker clean from issues that are not actionable. Please reach out if you have more information for us or you think this issue shouldn't be closed! 🙂 If you don't do so within 7 days, this PR will be automatically closed.

[joshribakoff-sm]

To me it appears the PR author addressed the feedback from @timdorr , and also signed the CLA as indicated by the bot, but @timdorr manually removed the label without clarifying why -- perhaps @timdorr could explain to the PR author what needs to be done to move this forward.

[timdorr]

No idea why that happened. Must have been a mis-click. Sorry about that! Label re-applied.

[joshribakoff-sm]

Thanks! 😆

Could this be merged or is it stale now/waiting on the PR author for anything else?

[timdorr]

@brophdawg11 Will this have an impact on the things you just did in #10336?

[brophdawg11]

@timdorr It shouldn't within the context of BrowserRouter. #10336 basically forks useNavigate based on whether we're in a RouterProvider or not - the implementation inside RouterProvider is now stable and also removes the activeRef stuff so switching to a RouterProvider "fixes" the issue (after 6.11 is released). This would still be relevant for BrowserRouter usages of useNavigate.

Let me see if I can follow up with @mjackson or @ryanflorence on getting this merged.

[brophdawg11]

🙌 Thanks for the PR @42shadow42!

I synced up with @mjackson and we're going to add back in the activeRef short circuit because in non-data routers on initial page load there's no history listener yet wired up to catch the navigation so it leads to broken code anyway. However the layout effect seems the correct fix for the parent/child issue in #8809. This also becomes a non-issue with https://github.com/remix-run/react-router/pull/10336 since RouterProvider apps will hit a different code path without any of the activeRef stuff.

I'm going to merge this into a branch of mine add some specific test scenarios and then will get it merged from there.