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

Upgrade react-redux to 6, adhere to React Context API #135

Merged
merged 2 commits into from
Mar 10, 2019
Merged

Upgrade react-redux to 6, adhere to React Context API #135

merged 2 commits into from
Mar 10, 2019

Conversation

ioanlucut
Copy link
Contributor

@ioanlucut ioanlucut commented Feb 25, 2019

In order to adhere to the new React context API.

Also, upgrades to the latest react-router-config beta version, adds the react-router-dom extra package as peer dependency, and upgrade all the other dependencies to their latest minor/major versions.

Also, drops the node 9 support, and focus on the LTE version of node (8, 10).

I got inspired from:

@ioanlucut ioanlucut changed the title ref #133: Upgrade react-redux to 6 Upgrade react-redux to 6, adhere to React Context API Feb 25, 2019
…eact context API.

Also, upgrades to the latest react-router-config beta version, adds the react-router-dom extra package as peer dependency, and upgrade all the other dependencies to their latest minor/major versions.
@AVVS
Copy link
Member

AVVS commented Feb 28, 2019

@ioanlucut thanks for the PR, this looks really good - only concern I have is that we reserve "store" property and it could potentially break the apps using it, what do you think?

@AVVS AVVS requested a review from aleksxor February 28, 2019 17:23
@ioanlucut
Copy link
Contributor Author

ioanlucut commented Feb 28, 2019

@AVVS thank you for having a look.
Do you mean this bit:

{({ store }) => <AsyncConnect store={store} {...otherProps} />}

?

@AVVS
Copy link
Member

AVVS commented Mar 2, 2019

yeah, this bit
if for some reason, store is passed as a prop to AsyncConnect in other props - it would lead to bugs

@ioanlucut
Copy link
Contributor Author

@AVVS I can rename this prop to be e.g. reduxStore or anything.
Please let us know about the review progress. We are waiting very much for a new release. Thank you!

@ioanlucut
Copy link
Contributor Author

@aleksxor I would appreciate a lot if you'd take some minutes to review this PR. Thanks!

@aleksxor
Copy link
Contributor

aleksxor commented Mar 6, 2019

the PR looks pretty good for me. though I have the same concern about prop naming. something like reduxConnectStore | _reduxConnectStore would make much smaller surface for possible collisions

…yncConnect to avoid possible collisions
@ioanlucut
Copy link
Contributor Author

@aleksxor thanks for the feedback and the approval. Waiting for the merge and release! :)

@ioanlucut
Copy link
Contributor Author

@AVVS When you find some time, please re-check! Thank you!

@ioanlucut
Copy link
Contributor Author

Hi @AVVS ! I am sorry for stressing you out, but I'd appreciate a lot getting this PR merged.
We are literally blocked by this release in order to go with redux6.
Thank you for your time!

@AVVS AVVS merged commit 5a72112 into makeomatic:master Mar 10, 2019
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.

4 participants