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

Add connectRouter + example #7

Merged
merged 4 commits into from
Dec 23, 2016
Merged

Add connectRouter + example #7

merged 4 commits into from
Dec 23, 2016

Conversation

dictions
Copy link
Contributor

Fixes #1

@dictions dictions changed the title Add connectrouter + example Add connectRouter + example Dec 23, 2016
import counterReducer from './counter'

const rootReducer = combineReducers({
count: counterReducer,
router: routerReducer,
count: counterReducer
Copy link
Owner

@supasate supasate Dec 23, 2016

Choose a reason for hiding this comment

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

Add trailing comma to pass eslint.

count: counterReducer,

@supasate
Copy link
Owner

Need to modify ConnectedRouter.js also

@supasate
Copy link
Owner

@dictions Need another little linting fix about trailing comma (#7 (review))

@dictions
Copy link
Contributor Author

@supasate weird, I couldn't get eslint to fail with or without the trailing comma.

@supasate
Copy link
Owner

Oh, I just noticed that I don't use airbnb rules in this project. My eyes worked like airbnb linting compiler lol.

@supasate
Copy link
Owner

LGTM. Thank you for your contribution!

@dictions
Copy link
Contributor Author

Of course! Mind bumping the version on merge?

@supasate
Copy link
Owner

Yes. I'll add some more details in the README before bumping version.

@supasate supasate merged commit c0fb82e into supasate:master Dec 23, 2016
@supasate
Copy link
Owner

Already released in v2.0.0-alpha.1 https://github.com/supasate/connected-react-router/releases

@wtgtybhertgeghgtwtg
Copy link

Why is it a higher-order reducer?

@supasate
Copy link
Owner

@wtgtybhertgeghgtwtg To initialize router state with current browser history. Additional benefit is users don't have to mount router reducer themselves coz it's automatically mounted.

@wtgtybhertgeghgtwtg
Copy link

wtgtybhertgeghgtwtg commented Dec 30, 2016

It complicates replaceReducer.

const newReducer = combineReducers(reducers);
store.replaceReducer(newReducer);

would have to be replaced with

const newReducerWithoutRouting = combineReducers(reducers);
const newReducer = connectReducer(history)(newReducerWithoutRouting);
store.replaceReducer(newReducer);

or, if you stored the result of the original connectReducer(history)

const newReducerWithoutRouting = combineReducers(reducers);
const newReducer = storedConnectReducer(newReducerWithoutRouting);
store.replaceReducer(newReducer);

@supasate
Copy link
Owner

I see your point about replacing reducer. Without higher order reducer, we've ever tried with following methods.

  1. Dispatching onLocationChanged in constructor of ConnectedRouter with the current history location.
    However, the initial rendered page will be the default state before transitioning to the page of current history location. Therefore, we need somehow to set initial reducer state with values in history object.

  2. We initially had an idea about creating a reducer creator that takes history object. Then, users call that creator and mount it to the root reducer.
    However, we found it complicated when most users separate the root reducer to another file and need to make a root-reducer creator to take history object in.

  3. Our next solution is to import history directly in our router reducer file to create initial state. However, that makes it hard to test because it's highly coupled with history object.

  4. We've reached the current solution to solve above issues by using higher order reducer.

(see our discussion here)

Therefore, I think, currently, I choose to trade-off with a bit complication of replacing reducer until we find a better solution. If you have any idea, please let me know.

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.

None yet

3 participants