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

[42shadow42]

Fixes #8809

[remix-cla-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 hello@remix.run.

Thanks!

- The Remix team

[timdorr]

Can you please revert the changes to the compact imports? They are intentional. This is explained here:

react-router/packages/react-router-dom-v5-compat/index.ts

Lines 1 to 48 in f3d87dc

	/**
	* Welcome future Michael and Ryan 👋, or anybody else reading this. We're doing
	* weird stuff here to help folks to migrate their React Router v5 apps to v6.
	*
	* So WTFlip is going on here?
	*
	* - We need to be able to run react-router-dom@v5 in parallel with
	* react-router-dom@v6
	*
	* - npm/node/whoever doesn't let an app depend on two versions of the same
	* package
	*
	* - Not only do we need to run two versions of the same package, we also need
	* this `CompatRouter` to *itself* depend on both versions 😬
	*
	* - When this package depends on "history" and "react-router-dom", those need
	* to be the `peerDependencies` that the application package.json declares.
	*
	* - That means this package can't depend on "react-router-dom" v6, but it needs to,
	* therefore it must ~become~ react-router-dom v6 👻
	*
	* - We could import from "../../react-router-dom" and get rollup to bundle it as part
	* of this package instead BUT ...
	*
	* - TSC needs to generate the types, and it has to derive the output paths from
	* the import paths. If we have a weird require *outside of this package* to
	* "../../react-router-dom" it's going to generate types from the common root
	* of all module paths (Which makes sense becuase what else would it do? It
	* needs to write the type files next to the source files so that typescript
	* can resolve the types for tooling in the same location as the modules).
	* Because tsc isn't as flexible as rollup, we have no control over this
	* behavior.
	*
	* - Therefore, we simply copy the v6 modules into this package at build time.
	* TSC won't come across any weird require paths (requiring outside of this
	* package with "../../") and everybody puts the files where everybody else
	* expects them to be.
	*
	* - The v6 `StaticRouter` found in `react-router-dom/server.ts` has a funny
	* require to *itself* ("react-router-dom") so we had to duplicate it in this
	* package because "react-router-dom" will point to the app's v5 version. We
	* can't change that require to a normal internal require ("../index") because
	* we generate two bundles (one for "react-router-dom" and one for
	* "react-router-dom/server"). If it were an internal require then SSR router
	* apps would have two copies of "react-router-dom", two contexts, and stuff
	* would break. We could stop doing two bundles in v6 "react-router-dom" and
	* deprecate the deep require if we wanted to avoid the duplication here.
	*/

[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]

⚠️ No Changeset found

Latest commit: 0f8205e

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]

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 #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.