react-router-dom -> react-router/dom

[timdorr]

I'm seeing a lot of issues come up relating to mismatched versions/instances of the Context pair. This is made more likely because we have two packages (react-router and react-router-dom), one of which is an implicit dependency (i.e., you should be installing only react-router-dom and react-router comes along for the ride).

The most common issue seems to be when someone is using both react-router-dom and a library that depends on just react-router. A version mismatch is handled by npm by giving each package their own copy of react-router, which will result in two Context instances. Right now we only have one version with the new Context API, but expect this to flare up more and more with new versions, especially if we bump the minor or major.

I would like to see about including the react-router-dom contents within the react-router package as a separate importable module. To ease the transition, I would suggest making it available at react-router/dom, so that the change is only one character and can be easily find-and-replace'ed.

This is just a preliminary thought, so I haven't figured out all the edge cases yet. But I wanted to discuss it further before proceeding to write any code. So, is it a great idea or the greatest idea?

[StringEpsilon]

I'm in favor.

Libraries aimed at both -dom and -native will be worse off after the transition is complete, but I think that can be justified by the orders of magnitude difference in popularity[1].

[1] 5000 dependents vs. 45 dependents and 2 million weekly downloads vs. 13k weekly downloads.

[timdorr]

We can always keep releasing react-router-dom indefinitely. It just wouldn't be needed anymore.

[mjackson]

A version mismatch is handled by npm by giving each package their own copy of react-router, which will result in two Context instances. Right now we only have one version with the new Context API, but expect this to flare up more and more with new versions, especially if we bump the minor or major.

I don't understand how a react-router/dom bundle would help this problem (though I really like the idea of ditching the monorepo and getting back to publishing a single package). Wouldn't you still have 2 context instances if one of your deps is using the router as well?

[pshrmn]

I don't think that you could fully ditch the monorepo; react-router-native would add react-native as a dependency to every application (unless the package was abandoned at which point there wouldn't be a need for a -dom qualifier).

[mjackson]

I don't think we should do react-router/dom, the main reason being that I do not want people using react-router-native to get react-dom as a peer dep. We don't peer-depend on react-dom in react-router-dom currently, but we could in the future.

The root of the problem is people depending on react-router and not react-router-dom or react-router-native. People should never depend on react-router or install it directly. It's just a dependency of react-router-dom and react-router-native.

This is mostly a problem that we (probably mostly me?) have created by telling people to install react-router and import stuff directly from react-router in our docs and examples. There is a helpful note in the react-router README about this, but we still do have quite a few docs telling people to import stuff from react-router directly. We need to get rid of those and make it a lot more clear that our expectation is for people to never depend directly on react-router.

I'll comb through the docs today and sniff out all the faulty references. Also, I'll probably try to move all the docs to the root of the repo to make them a little more prominent and let people discover them more easily.

[timdorr]

This is something we deal with on react-redux, where we support both dom and native, proxying the batchedUpdates API from each as a batch function publicly.

The good news is there is a fix coming, the bad news is only recent versions of npm support it: reduxjs/react-redux#1390

[StringEpsilon]

You could just make both the native and dom packages vendor what's currently react-router, instead of depending on it. (I don't know if you'd actually need the react-router package at that point or not. Does react have popular other targets?)

That would get rid of the issue entirely, but it has some mean downsides. I'm mostly suggesting it for completeness sake.

[timdorr]

You'd definitely end up with mismatched context instances in that case.

[thclark]

Not a maintainer and I don't have huge expertise at all here, so take with a pinch of salt.

If my understanding (after a painful 7 hours understanding this and filing this issue) is correct, this is much more of a communications issue than a technical one.

The entire web is scattered with examples where react-router is installed directly, ignoring react-router-dom, which must seriously obfuscate this issue. In a fairly aggressive google, here is the only reference I can get to a member of the RR team actually saying we should be using RRD instead of RR almost all the time (I assume the same is true of RRN for native uses?).

Even major libraries like connected-react-router encourage importing directly from RR in their examples.

So just thinking outside the box, would

• an afternoon of finding and correcting / filing issues with the most readily available tutorials / related libraries on the web, and
• A docs section for library developers suggesting use of only RRD or RRN, and that they be installed as external/peer dependencies

go most of the way to fixing this?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[overlookmotel]

An alternative solution would be to put the Context initialisation in a separate module react-router-context. react-router could then depend on react-router-context and import the Context from there.

Regardless of whether other modules depend on react-router or react-router-dom, or what versions, there'll only be one react-router-context version, and so it'll get hoisted to the top of the dependency tree => no mismatches.

A bit hacky, but would solve the problem with no breaking changes.

[overlookmotel]

NB I know this issue has been closed, but it's still linked to from the Roadmap issue #6885 so I thought it worth raising.

[mjackson]

I need to update the public roadmap. Thanks for the reminder, @overlookmotel.