use-react-router icon indicating copy to clipboard operation
use-react-router copied to clipboard

Ask people to use 5.1 instead?

Open mjackson opened this issue 6 years ago • 2 comments

Hi @CharlesStover 👋

I think this package has been causing some problems with people who are using React Router and getting 2 different copies of our context object. The problem is that you depend on react-router, so you have one instance of our __RouterContext object. But react-router-dom and react-router-native also depend on react-router, so if the 2 copies don't match up exactly then people end up using a different instance of context which breaks things.

It's hard to reproduce this behavior because package managers vary in how they decide to hoist dependencies and deduplicate them. But we've seen a few issues crop up in the main router repo that seem to indicate we have an ecosystem problem (see https://github.com/ReactTraining/react-router/issues/6755).

The solution is that you should never depend on react-router directly. Instead, you should depend on react-router-dom or react-router-native, depending on which platform you're targeting.

But there's also some good news, and that is that we shipped some hooks yesterday in v5.1, so people shouldn't need use-react-router anymore. So an even simpler solution would be to deprecate this package with a notice message that encourages people to use v5.1 instead. This would also be ideal for us since we never intended 3rd party libs to use our context object (hence the __ prefix).

Thanks!

mjackson avatar Sep 25 '19 18:09 mjackson

Hi @mjackson. Thanks for your great work!

While I encourage people to use the first-party hooks, I don't want to deprecate this for users who are unable or unwilling to upgrade to 5.1 or refactor their existing code. A helpful balance may be to set the peerDependency to ~5.0.0 instead of ^5.0.0 and have this repo's documentation encourage users to read the react-router's hooks documentation as a preferred alternative.

I'll leave this issue open until I think of the optimal solution for users, and I encourage readers to migrate going forward.

quisido avatar Sep 25 '19 19:09 quisido

I encourage readers to migrate going forward.

No better way to encourage people than by deprecating your package :) They'll still be able to use it. npm just lets them know at install time that they should be using something else. But they are free to ignore the warning and use it anyway.

If you don't want to deprecate, would you at least please put a link in the README to this issue as a "known issue" so that people know there are risks involved with using this package?

mjackson avatar Sep 25 '19 21:09 mjackson