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

fix(dependencies): moved redux, react-redux and react-router-redux to peerDependencies. #2760

Closed
wants to merge 6 commits into from

Conversation

kopax
Copy link
Contributor

@kopax kopax commented Jan 11, 2019

HI @fzaninotto. Sorry for the delay, we have not received a notification when you reopened #1683.

I have seen that you already moved part of the dependencies into peerDependencies.

redux should also be in peerDependencies, so does react-redux.

I am not sure for react-router-redux, does this package use a store reference or does it just export a reducer to be used somewhere?

By doubt and because it would solve all problems, I also added it as a peerDependencies.

@djhi djhi added this to the 2.7.0 milestone Jan 14, 2019
@Kmaschta
Copy link
Contributor

Isn't this change a breaking one?

@kopax
Copy link
Contributor Author

kopax commented Jan 14, 2019

@Kmaschta you are absolutely right, I forgot to mention it, you should add a note in CHANGELOG.md for users.

Moving a dependencies into peerDependences and dependencies will change nothing in development (you must have it at some point). In production, it will require the user to have our list of peerDependencies installed in it's dependencies or devDependencies.

If the user does not have redux in it's devDependencies or dependencies, then it will break.

If the user have redux or at least 2 sub depencencies that depend on redux, then it will work.

note: 2 sub dependencies that depend on redux means npm will try to dedupe, deduped package will appear in the root node_modules folder so redux will be resolvable. If one dependencies depend on redux, it will not be in the root node_modules but in the sub modules node_modules folder and it will not be resolvable

@fzaninotto fzaninotto removed this from the 2.7.0 milestone Jan 15, 2019
@fzaninotto
Copy link
Member

Thanks!

This really is a breaking change, so we won't merge it before 3.0. And we're waiting for react 16.8 and material-ui 4.0 to break BC for good, so this PR won't be merged before a few months at least.

@kopax
Copy link
Contributor Author

kopax commented Jan 15, 2019

Sure. About react-router-redux, the version 5.0.0-alpha.9 was never released. Instead they renamed the package connected-router.

I can't find the source code of 5.0.0-alpha.9, but https://github.com/supasate/connected-react-router/blob/master/src/reducer.js show they are doing the reducer in here. It should be a peerDependencies.

Could we, at the same time. replace [email protected] with connected-react-router? It work the same way, just the import and the dependency change.

At the same time, we could also include the update of redux, react-redux, react-router-dom, react-router.

I've already played with all these versions and none of theme are breaking changes.

@fzaninotto
Copy link
Member

see #1999 for react-router-redux

@kopax kopax closed this Mar 25, 2019
@kopax kopax deleted the master branch March 25, 2019 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants