search

faceyspacey / redux-first-router-link

<Link /> + <NavLink /> that mirror react-router's + a few additional props
MIT License
55 stars 33 forks source link

Support react-redux 6 #108

Closed circlingthesun closed 5 years ago

circlingthesun commented 5 years ago

Fixes #101

ScriptedAlchemy commented 5 years ago

Thanks! For everyone’s learning, including my own - can tell us a bit about what happened and changed it to in the description.. for release notes.

Does this need any updates to tests

mungojam commented 5 years ago

Nice, looks much simpler now

ScriptedAlchemy commented 5 years ago

Just waiting on more details so others can benefit. Many will need to port stuff to redux 6. Especially me. So it would be great to have some information about why routesmap needs to come in and such.

Remember, this is helivaly maintained by the community. The often look to other PRs and issues on how to create PRs in our other repos

cdoublev commented 5 years ago

routesMap was read from context before, ie. from the legacy React context received as the second argument of a functional component or from an instance property of a React.Component. As of react-redux@v6.0.0, the store state could not be read from legacy context anymore. It has to be read from a connected component via the received props.

Any library that attempts to access the store instance out of legacy context will break

https://github.com/reduxjs/react-redux/releases/tag/v6.0.0

Please take note of how this PR is changing the render tree of a <NavLink> with a duplicated wiring to store (vs #107). I don't know what the perf costs would be but I don't see any pros of doing it this way.

circlingthesun commented 5 years ago

What @cdoublev said. It looks like we working on a fix at the same time. Avoiding the use of the already connected Link inside an also connected NavLink is probably better. @ScriptedAlchemy you should probably accept #107 as @cdoublev was first. I'll just quickly make some comments over there.

GuillaumeCisco commented 5 years ago

I also experience this kind of error while upgrading to react-redux 6.0.0 Based on both of your PRs https://github.com/faceyspacey/redux-first-router-link/pull/108 and https://github.com/faceyspacey/redux-first-router-link/pull/107, I created this one https://github.com/faceyspacey/redux-first-router-link/pull/113 for unifiying ideas.

It fixes https://github.com/faceyspacey/redux-first-router-link/issues/101 without side effect. Feel free to update your PRs with ideas in https://github.com/faceyspacey/redux-first-router-link/pull/113, @cdoublev do not hesitate to copy/paste the code. Thank you both circlingthesun and @cdoublev for your work. I simply rearranged it :)

I can confirm this upgraded code work in our setup (Server side rendering with streaming and caching over http2 secure).

I hope @ScriptedAlchemy will have time to review this and merge it quickly in master. It is cryptic in our upgrade process.