Invariant violation related to react-router / react-router-dom non-peer dependency

[thclark]

First up. I'm known for concise and helpful issues. But this one gets a bit tortuous. Sorry about that. Hopefully the proposed solution is simple.

Overview

I have an issue closely related to, but not the same as #3018 and its dupe #3019. It's caused by the way in which react-router and react-router-dom access their internal context (the authors are humming about a better way here but that's too late to save my Monday :( .

#3078 is very closely related (almost certainly the same issue), but has been closed without identifying the root cause, and doesn't highlight the case where you actually import react-router or react-router-dom elsewhere outside <Admin>. It's left quite a few people hanging, not understanding why they can't have react-router-dom in their package.json.

What you were expecting:
To be able to use a <Link> component, from react-router-dom, in a page whose <Route> is outside of <Admin>, but in a valid router.

What happened instead:
I get a variety of errors similar to:

Invariant Violation: You should not use <Switch> outside a <Router>

even though things are inside valid routers.

Steps to reproduce:
This seems to be very much an edge case, dependent on npm's rules for resolving package dependencies, but I believe the following steps should reproduce:

• Add react-router-dom (I used 5.1.2) to your app's package.json
• Place your <Admin .../> component in a <Route/>, which itself resides in a <Switch/> inside a valid <ConnectedRouter/>.
  • So far so good, this works. The router used by the Admin doesn't conflict with the outer Router even though they're nested, so you can happily use Admin alongside other stuff.
• Render a <Link> in a different page outside the admin route.
  • This now breaks, because to do this you have to install react-router-dom explicitly

I'm sorry, to make this work on a prototype deadline I just ripped out all <Link>s and references to react-router-dom elsewhere on my site, so it's chunk of work to go back. But I expect I'll encounter this again so the next time, I'll attach my package-lock.json to try and help show what's going on.

In depth

In the case of #3018, there was a clash of version between react-router and react-router-dom - put simply, they couldn't see each other's routes. That was back when RA was using v4.x.x of RR and RRD.

This commit back in June increased RA's major version of these dependencies to 5.x.x... meaning they now use a context pair internally to decipher what routes they're on.

The upshot of it is: you need RR/RRD to be using the same context instance throughout the entire app. Multiple contexts arise most commonly from having multiple installations (sometimes even of the same version!).

The same 3018 problem, in effect, is now happening in cases where multiple libraries that provide <Route>s import a different instance of react-router's context.

Workaround

To break out of this, you either:

• have to do the next steps in CustomApp.md and completely remove Admin altogether (fully customised app, which kinda defeats the point!)
• have to remove all uses of react-router-dom everywhere else in your library, delete react-router and react-router-dom from your package.json, rm -rf node_modules, rm package-lock, npm install. Not fun if you're trying to also render a landing page or similar!

Proposed solution

Is pretty simple. Hurrah!

• Import from react-router-dom, instead of react-router throughout RA. It's essentially identical (it re-exports everything from react-router, with extras). This is in-line with the (admittedly ridiculously hard to find) recommendation of the RR/RRD team that we should almost certainly be using RRD directly, and stops multiple contexts arising where RA imports from RR, but OtherCode has to import from RRD (i.e. stops RA from preventing third party libraries from using RRD components).

• Alter react-router-dom and react-router to be peer dependencies, rather than full dependencies, similar to this fix from Samson Crowley, which avoids risk of there being multiple instances of these libraries installed by npm (and therefore multiple contexts) if other libraries are doing the same hard exports, or where users are struggling to migrate to RA from existing apps.

Environment

• React-admin version: 3.0.0.beta-0
• Last version that did not exhibit the issue (if applicable): prior to this, I assume.
• React version: 16.10
• Browser: Chrome
• Stack trace (in case of a JS error):

[fzaninotto]

Thanks for the detailed (and very clear) explanation.

For the peer dependencies, the fix was already committed in #3763 - I mean, the fix we're willing to accept without making the installation more painful.

As for using react-router-dom instead of react-router in react-admin, would you mind opening a Pull request for that?

[thclark]

@fzaninotto happy to, but it'll be a week or so as I'm on anotehr project for a little while :)

[thclark]

OK @fzaninotto PR complete and testing for the imports aspect.

It's worth noting that #3763 is not a fix to this issue. It tidies up the imports nicely (and makes it less likely that a version mismatch will occur within RA itself) but the dependency is simply moved into the react-admin package.

However, I understand that adding peer dependencies makes installation a little harder, which is not aligned with the "out of the box" philosophy of react-admin.

The options for progressing are either:

1. Add that step to installation, or
2. Label this bug as wontfix

The choice is yours as maintainer!

I'll totally understand either way. For my $0.02, though, not fixing issues of this nature is a step away from being able to claim "Can be included in another React app" (which is probably a bit much if it makes routing the rest of the app app extremely hard/impossible/flaky).

What do you think?

[kopax]

First of all, thanks for your PR for react-router optimization.

I won't go into the peerDependency debate again but just to help @thclark:

1. you can fork react-admin and create @thclark/react-admin and move all the dependency you don't want in peerDependency, you will still depend of core packages. You will still be compatible with ra related packages, you will still support ra-upgrade (there's nothing in react-admin package).
2. Otherwise, you could just stop using react-admin and install core packages manually in your project, that's completely valid.

We will use strategy (1) for our applications and we will use strategy (2) for our react-admin related and distributed modules.

I am still waiting for a release of V3 that include #3763, would you mind doing a beta.1 soon @fzaninotto ?

[thclark]

Aha, ok thankyou @kopax that's a pretty robust approach, I think. Those ways forward, plus this issue being searchable, should reasonably cover this edge case for other users.

[kopax]

You are welcome, this should be explained in the documentation at some point.

[fzaninotto]

fixed by #3825