Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mark Redux as an optional peer dependency #1498

Closed
wants to merge 1 commit into from
Closed

Conversation

xty
Copy link
Contributor

@xty xty commented Jan 13, 2020

TL;DR: With the introduction of @reduxjs/toolkit, we can now use it in place of redux, thus making both peer dependencies optional.

As suggested by @markerikson in reduxjs/redux-toolkit#238 (comment),

One other small downside is that React-Redux says it wants Redux as a peer, and logs a warning if only RSK is installed.

Currently, this warning may very well give the false impression that @reduxjs/toolkit must be installed alongside redux for it to work properly.

Ideally, an UNMET PEER DEPENDENCY warning should by displayed iff neither redux nor @reduxjs/toolkit is installed, but neither npm nor yarn has a way to specify such a requirement AFAIK. That leaves us no choice but to mark both as optional (similar to the react-dom-vs-react-native situation), if we are to avoid being misleading.

In the future, leaving redux as is might pose a bigger problem still with npm v7, which will install peer dependencies automatically, as outlined in the npm CLI Roadmap among other places:

Proper peerDependencies Support

Part of the installer rewrite involves taking a fresh look at how peerDependencies are handled (and, quite often, not handled) to maximize the cases where the CLI does the right thing. Since npm v3, peerDependencies were not installed by default, putting the onus on users to resolve them. We'll be bringing back this functionality in version 7.

@netlify
Copy link

netlify bot commented Jan 13, 2020

Deploy preview for react-redux-docs ready!

Built with commit a7bd68f

https://deploy-preview-1498--react-redux-docs.netlify.com

@timdorr
Copy link
Member

timdorr commented Jan 13, 2020

What about the fact that RTK requests redux@^4, whereas React Redux is compatible with redux@^2-4? I suppose we should bump up to the same ^4 dependency.

Another option: What if RTK re-exported React Redux? Sort of akin to react-router-dom re-exporting all of react-router.

@markerikson
Copy link
Contributor

RTK is supposed to be independent of React-Redux and usable with any UI layer, in the same way that the Redux core is. So no, RTK can't depend on React-Redux at all.

Right now, using RTK + React-Redux shows a warning at install time, but we get away with it at runtime because of how package managers and bundlers end up handling things (nested dependencies get flattened to the root of node_modules, and you can have import statements for transitive packages without explicitly listing them in package.json because of that flattened hierarchy).

In terms of the actual source code, the only true dependency React-Redux has on Redux is the two places where we import bindActionCreators (

import { bindActionCreators } from 'redux'
and
import { bindActionCreators } from 'redux'
).

The store shape aspect (ie, expecting store to be an object like {dispatch, subscribe, getState}) is more of an implicit / duck-typing kind of thing.

So, strictly speaking, React-Redux itself is indeed compatible with redux@^2, because all it needs is bindActionCreators + something store-like.

If we were to copy-paste bindActionCreators over, we could remove the dependency on Redux entirely, and mark both Redux and RTK as purely optional. That said, not sure if it's the right approach or not.

@markerikson
Copy link
Contributor

markerikson commented Mar 30, 2021

FWIW, bindActionCreators is all of 25-ish lines long. I vote we just copy-paste it over. We could even make the shape checks be dev-only to shave a few bytes.

For that matter, in theory you should only get that if you're using connect anyway. If you're only using the hooks API, that should never get imported.

From there we add RTK and Redux as optional peerDeps, but all you really need is a Redux-store-shaped store passed to Provider.

@timdorr
Copy link
Member

timdorr commented Mar 30, 2021

Just did that over in #1705

@xty
Copy link
Contributor Author

xty commented Apr 9, 2021

Well done, guys

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants