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

connected-react-router -> redux-first-history #1947

Merged
merged 1 commit into from
Nov 1, 2022
Merged

Conversation

krispenney
Copy link
Collaborator

@krispenney krispenney commented Nov 1, 2022

connected-react-router is most likely out of maintenance supasate/connected-react-router#579, with it being the blocker the remaining major dependency upgrades (react, redux, history)

Most others on the repo recommend switching to https://github.com/salvoravida/redux-first-history (which is marketed as a "better alternative to connected-react-router"), this PR does exactly that.

@krispenney krispenney changed the title connected-react-router -> redux-first-router connected-react-router -> redux-first-history Nov 1, 2022
@krispenney krispenney marked this pull request as ready for review November 1, 2022 17:44
@krispenney
Copy link
Collaborator Author

I also want to note, I think the whole api here (both the old and new, my goal here wasn't to change it much yet) is super weird. It kind of puts the burden of the dependency on both the framework and the app, while providing no real benefit.

However now that it's just a reducer directly now, I think maybe there's a way we can hide this completely from the app (as it's just another reducer to combine). Really all we should really expose to the app is some kind of configuration interface imo.

Copy link
Collaborator

@keeferrourke keeferrourke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems straightforward. Is this a breaking change for misk apps?

@keeferrourke
Copy link
Collaborator

However now that it's just a reducer directly now, I think maybe there's a way we can hide this completely from the app (as it's just another reducer to combine). Really all we should really expose to the app is some kind of configuration interface imo.

This is an interesting idea. What would this configuration look like?

@krispenney
Copy link
Collaborator Author

Is this a breaking change for misk apps?

Yes it is because of my comment above, this is basically how it would look in an example app.
https://github.com/cashapp/misk-web/pull/1947/files#diff-6f5a489992d29756a72f1270e0764752d228278c170f85513dec2a7717ea757aR65-R70

So I'm thinking of bundling this + the rest of the big upgrades (react, redux) in a v0.6 release.

This is an interesting idea. What would this configuration look like?

The router itself doesn't really have many configuration options: https://github.com/salvoravida/redux-first-history#options, so I don't think there's much to configure there. I'm thinking more of reversing the flow in a sense. So for example, rather than us calling to the app with the routerReducer and having them mix it into their apps reducers, we receive the apps reducers and mix in the routerReducer there.

@krispenney krispenney merged commit 5b01487 into master Nov 1, 2022
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.

2 participants