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

#1792 candidate: Make combineReducers shape-agnostic #1817

Closed
wants to merge 2 commits into from

Conversation

borkxs
Copy link
Contributor

@borkxs borkxs commented Jun 17, 2016

Candidate for #1792. Adds options parameter to combineReducers which can include get, create, and keys methods. There is no set method because we can just use create instead.

Adds a few new warnings and errors relevant to these methods without altering the old warnings and errors. The isPlainObject warning now just checks for typeof state === 'object', so it still warns when something like a Number is passed in.

The keys methods is for iterating over the state keys, which is needed to check for unexpected keys. It can be excluded and will simply warn if the result of the default Object.keys(...) doesn't match the reducer shape.

Also added a param to getUnexpectedStateShapeWarningMessage to pass in the already computed reducerKeys instead of recalculating them from the reducer.

@borkxs
Copy link
Contributor Author

borkxs commented Jun 17, 2016

It can't find the immutable module I added to devDependencies, I'm guessing node_modules are cached? i'm an idiot

@timdorr
Copy link
Member

timdorr commented Jun 26, 2016

I'm 👎 on this. combineReducers is a convenience function, not a requirement for Redux to operate. If you want to use another library to store the results of your reducers, I would build bindings for it and implement your own version of this function. This adds too many abstractions.

@timdorr
Copy link
Member

timdorr commented Jul 19, 2016

Closing due to lack of input. I'll reopen if some support for this can be drummed up (of course, I'm still against it, sorry :( )

@markerikson
Copy link
Contributor

@borkxs , @timdorr , @jimbolla , @gaearon : I'm seriously interested in revisiting this idea. Tim, I see you closed this a year ago as a "eh, we don't want this" decision. On the other hand, in November you said at #1768 (comment) :

I'm a fan of composition vs. expanding an API. If we could break up combineReducers and let users put it back together however they want (along with maintaining the existing API), that would be the most scalable option.

Is the approach of defining get/set/create functions sufficient for "breaking things up" in your view, or did you have something else in mind?

I personally am in favor of adding this, and also adding options to make the key warnings and dropping of unhandled slices configurable (related to #2427 ).

@timdorr
Copy link
Member

timdorr commented Aug 3, 2017

I didn't have any specific API or structure in mind, just the overall goal of a composable combineReducers. I'm happy to examine it, but we should have scalability in mind. If it can solve the issues of #1768 at the same time, that's even better.

@eddyw
Copy link

eddyw commented Aug 8, 2017

I had an idea some time ago how to implement some kind of 'utils' in the reducers, so they would work either with ImmutableJS or without it.
I wrote something similar but not exactly to this:

const redux = require('redux')
const ini = {
	count: 0,
	nested: {
		sample: 1
	}
}
const countReducer = (state = ini, action) => {
	const u = action.utils
	switch(action.type) {
		case 'increment':
			console.log('Current state tree:', u.getState())
			return u.merge(state, {
				count: u.get(state, 'count') + action.payload,
				nested: u.merge(u.get(state, 'nested'), {
					newPropInNested: 2
				})
			})
		default:
			return u.object(state)
	}
}

const addUtils = api => {
	const utils = {
		get: (obj, prop) => obj[prop],
		object: obj => obj, // Use: obj => Immutable.Map(obj) for ImmutableJS, for instance
		merge: (...objs) => Object.assign({}, ...objs),
		getState: () => api.getState() // Additionally allow access to entire state
		// other helpers or utils
	}
	return dispatch => action => {
		const a = Object.assign({}, action)
		a.utils = utils
		dispatch(a)
	}
}
const store = redux.createStore(countReducer, redux.applyMiddleware(addUtils))
store.subscribe(() => {
	console.log('State tree changed:', store0.getState())
})
store.dispatch({ type: 'increment', payload: 10 })

However, it relays in the fact that I will never send a 'utils' prop in the action object to the dispatch method.

My proposal would be to make the middlewareApi available as third parameter in the reducers, so it can be extended instead of changing anything in the combineReducers fn:

const countReducer = (state = null, action, middlewareApi) => { // middlewareApi.utils = {...}
	switch(action.type) {
		default:
			return state
	}
}
const middleware = middlewareApi => {
	middlewareApi.utils = {
		get: (obj, prop) => obj[prop],
		merge: (...objs) => Object.assign({}, ...objs)
	}
	return dispatch => action => dispatch(action)
}
const store = redux.createStore(countReducer, redux.applyMiddleware(middleware))

This way, it even exposes getState() method to the reducer, so it can access the state tree (for any reason somebody would like to do that)

@SeeThruHead
Copy link

would love to see this

reduceReducers(
  combineReducers({
    meta,
    contacts,
    content
  }),
  combineReducers({
      blah,
      foo,
      baz
  })
);

i've completely defined my state tree, but i have no state, this is really odd to me

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.

5 participants