Update react-router to v6

[devonpmack]

Description

Fixes #2275

Updates React router to v6 to unblock the rest of Shopify on upgrading react-router and react-router-dom.

[devonpmack]

I could not find equivalents for these types

[BPScott]

(I'm going to take a while to get to the review bit, bare with me while I rant)

I think trying to reexport 90% of a public package, and then add on 3 extra things is silly. I think trying to make everybody use react-router's API through the reexports of @shopify/react-router just makes things more confusing. It makes teaching people "we use react-router as a router" harder. It makes reading upgrade notes / migration guides between major versions of react-router harder and keeping dependencies updated becomes pointless churn.

The web-foundations teams of yore went through a phase of trying to whitelabel everything - instead of using some public package you should use @shopify/SOMETHING! Which was a neat idea when the team was able to focus on keeping these foundational technologies up to date and innovate and we wanted to bet that we could do better than the JS ecosystem. In some places (like graphql tooling) that was true at the time. These days however the ecosystem has plugged ahead while much of quilt has stayed where it was.

These days I reject that idea of being a central authority. We shouldn't be embracing and extinguishing react-router by making people use @shopify/react-router instead. We should show that we have some useful complementary utilities rather than trying to abstract over react-router entirely.

---

This PR's contents is great, but by bumping the react-router-dom version number and adjusting the list of stuff we re-export from react-router-dom, we perpetuate the old status-quo of hiding the fact that react-router is doing all the heavy lifting. I think that abstraction ultimately hurts understanding and that consumers should be referencing react-router-dom directly in the majority of cases.

What would you think to a new approach? What if we stopped reexporting stuff from react-router-dom, and instead expected consumers to reference react-router-dom where possible.

What if in addition to the current state of this PR we also did the following:

• Remove react-router-dom as a direct dependency. Add it to the peerDependency and devDependency arrays instead
• Delete the @types/react-router-dom dependency as it now ships its own types
• Remove all the reexports from react-router-dom in index.ts.
• Update the README.md to remove references to reexports from react-router-dom, and make it clear that the 3 components are to be used in conjunction with react-router-dom and that other routing should look at its documentation.
• Update the changeset to to talk about how we no longer reexport a bunch of stuff from react-router and that consumers should access Route/Switch/etc from react-router-dom instead

[julianguyen]

Are there specific or immediate blockers if this PR doesn’t get merged? Does this prevent current work from being completed?

[devonpmack]

@BPScott Thanks so much for the context and response. I fully agree that this package re-exporting react-router-dom makes things a lot more complicated and increases the maintenance burden. I made the changes 🙏

[devonpmack]

@julianguyen

Are there specific or immediate blockers if this PR doesn’t get merged? Does this prevent current work from being completed?

Nope, just need it so that we can upgrade to react-router v6 in our repo so that we're up to date with dependabot

[BPScott]

I just remembered #1673 where I talked about the idea that Link isn't a useful component. It doesn't handle external urls - only internal urls, so the current behaviour of Link isn't ever usable in practice.

To have a Link component that plays nice with Polaris and handles external links is trivial to implement, and is demonstrated in Polaris's documentation. I don't think there's any value in having polaris-specific logic in this package.

@devonpmack: I reckon we should delete the Link component too.

[devonpmack]

@BPScott Are these flaky tests? Seems like they pass after re-running

[BPScott]

react-testing being flakey is concerning, but it's unrelated to this PR. For this PR we can ignore that and they pass with a rerun. I'm going to give this another once-over, but I think we're close to being good to go.

Ping @melnikov-s - as he added the test destruction stuff. Sergey could you take a look at what might be causing these stale promise tests to intermittently fail: https://github.com/Shopify/quilt/actions/runs/3147770668/jobs/5128584382 as they were introduced in #2352. Test flake because of the testing framework itself feels very scary

[BPScott]

Yay! I think this looks good to me.

I'd like one other person on the @Shopify/admin-fed-infrastructure team to take a look and approve to double check my contributions to this PR too.

Particularly confirm that the README explains usage, and the changeset entry explains how people would go about migrating.

[melnikov-s]

Ping @melnikov-s - as he added the test destruction stuff. Sergey could you take a look at what might be causing these stale promise tests to intermittently fail: https://github.com/Shopify/quilt/actions/runs/3147770668/jobs/5128584382 as they were introduced in #2352. Test flake because of the testing framework itself feels very scary

I saw this before and thought I'd fixed it, guess not. It only happens with React 17 for some reason and it's not a result of library code in #2352 but rather the test the was introduced in that PR. The tests creates a hanging promise which will fail consistently without the changes in #2352 but for some reason fails intermittently and rarely with the changes and in React 17 only. It will be difficult to determine the source of the issue as I can't reproduce it locally. Knowing that it's the test we can maybe disable it when process.env.REACT_VERSION is 17

[julianguyen]

Looks good!

[devonpmack]

@BPScott Is this good to be merged? Just want to confirm!

[BPScott]

@devonpmack yep merge this when you're happy.