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

Expose router reducer #150

Merged
merged 5 commits into from
Nov 3, 2018
Merged

Conversation

sgal
Copy link
Contributor

@sgal sgal commented Sep 30, 2018

I created this PR to follow up on #132 and #140. One is closed and other one is stale, so hopefully this one gets to be merged

What kind of change does this PR introduce?

Refactoring and bugfix.

What is the motivation behind this change?

Align public API with react-router-redux and allow various integrations (redux-loop, rematch, etc.). As a consequence - fix for #133.

Did you add tests for your changes?

Yes, existing tests are updated and new one for #133.

Did you update the documentation?

Yes, both plain and immutable structure docs are update. All examples are also updated.

Does your PR introduce a breaking change?

Yes, public API is changed so major release is needed for this.

Fixes #133 #104 #79 . Closes #140

@schnubor
Copy link

schnubor commented Oct 4, 2018

2 PRs for such an essential feature and still none of them is merged, meh.

@supasate
Copy link
Owner

This is a really great PR! Due to it will change the way we use this library, it'll be a major update. I'd like to make sure that I spend enough time to check it thoroughly (I'll try it this or next weekend). In the meantime, I'd like the community to help check it if it breaks anything you're using or not if you migrate to this one.

@sgal
Copy link
Contributor Author

sgal commented Oct 20, 2018

I've tested this this PR in our app (30+ routes, 10k+ modules) and it works as expected. Did you have time to test it @supasate?

@YUANLIN327
Copy link

@supasate , it's been more than two weeks since you mentioned about checking the update. Would you please merge it ASAP?

@Eomerx
Copy link

Eomerx commented Nov 2, 2018

when dou you guys will merge this pull request and publish a new tag? There is no point to return new state every time an action is dispatched.

@sgal
Copy link
Contributor Author

sgal commented Nov 2, 2018

@Eomerx I’m wondering about the state myself. Seems like @supasate completely forgot about this repo. Which also brings a lot of questions regarding reliability of this package.

@supasate
Copy link
Owner

supasate commented Nov 2, 2018

I'm going to check on it this weekend (finger crossed). I'm super busy last month. Sorry about that.

@supasate
Copy link
Owner

supasate commented Nov 3, 2018

The logic looks solid and It works like a charm! I really appreciate your end-to-end contribution for the description, codebase, test, examples, and documentation. Thank you for your patience in my very slow response!

@supasate supasate merged commit 9632ab4 into supasate:master Nov 3, 2018
@supasate
Copy link
Owner

supasate commented Nov 3, 2018

Released in v5.0.0!

@sgal sgal deleted the expose-router-reducer branch November 3, 2018 16:37
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.

[Performance]: Redux state is re-created on EVERY action
5 participants