Update argument type of reducer produced by combineReducers #2115

Merged
merged 3 commits into from Nov 23, 2016

Projects

None yet

4 participants

@nickarora
Contributor

Addresses #2114

Updates CombineReducers flow-type

@nickarora
Contributor

tagging @gcanti and @aaronjensen

@gcanti
Contributor
gcanti commented Nov 21, 2016

Using $Shape makes combineReducers unsafe:

// test_combineReducers.js in flow-typed
type State = {
  a: AState,
  name: string,
  age: number
};


const reducer1: Reducer<State, Action> = combineReducers({
  a: reducerA,
  name: reducerName
  // missing age reducer but no error
})

This can become problematic if you are trying to completely wipe out your store.

What do you mean? The root reducer is not supposed to be called "by hand"

rootReducer({}, someAction);
@nickarora
Contributor
nickarora commented Nov 21, 2016 edited

The root reducer is not supposed to be called by hand

@gcanti See Dan Abramov's response here where he details a recommended approach to resetting the store. That would be the use case. In his case, he names it "appReducer" but it is the same idea.

@gcanti
Contributor
gcanti commented Nov 21, 2016

@nickarora note that in the linked approach appReducer is no more the root reducer, hence it can be called by hand. Also appReducer is called with undefined rather than {}. Maybe a better option, without losing type safety, would be

declare function combineReducers<O: Object, A>(reducers: O): Reducer<$ObjMap<O, <S>(r: Reducer<S, any>) => S> | void, A>;
@nickarora
Contributor
nickarora commented Nov 22, 2016 edited

@gcanti I've pushed up a change that should address your type-safety concerns while still allowing state to be reset using their default initialValues -- as dan explains.

@aaronjensen
Contributor

@nickarora I think I may have misled you on this a little when we talked earlier. If Reducer<S, A> is meant to be a general Reducer function then we shouldn't do this. It should take (S | void, A) as its arguments.

The result of combinedReducer, however, is different. It is actually less strict than Reducer<S, A> in that it allows the $Shape<S> | void as an argument.

Basically, if you pass undefined into the combined reducer, it defaults to {}. It then grabs the previousStateForKey (which can be undefined) and passes that to the child reducer which should handle undefined with a default.

Adding void to the first argument of the Reducer<S, A> type is a good change to make, but to fix this actual problem you'll want something like this:

declare type CombinedReducer<S, A> = (state: $Shape<S> | void, action: A) => S
declare function combineReducers<O: Object, A>(reducers: O): CombinedReducer<$ObjMap<O, <S>(r: Reducer<S, any>) => S>, A>;
@nickarora
Contributor

Sounds reasonable to me -- I'll let @gcanti weigh in before making any other changes

@gcanti
Contributor
gcanti commented Nov 22, 2016

Not sure why $Shape is involved, do you want to partially reset the store?

Beware: $Shape is lenient with both null and undefined

type O = { name: string };
const x1: $Shape<O> = undefined // <= ok
const x2: $Shape<O> = null // <= ok

Maybe we should exclude null explicitly

declare type CombinedReducer<S, A> = (state: $Shape<S> & {} | void, action: A) => S;
declare function combineReducers<O: Object, A>(reducers: O): CombinedReducer<$ObjMap<O, <S>(r: Reducer<S, any>) => S>, A>;

Then you can write

import type { Store } from 'redux'
import { createStore, combineReducers } from 'redux'

type State = {
  a: number,
  b: number
};

type Action = { type: 'RESET' };

const appReducer = combineReducers({
  a: (s = 1) => s,
  b: (s = 2) => s
})

const reducer = (s, a) => {
  if (a.type === 'RESET') {
    // return appReducer(null, a) // <= error
    // return appReducer(undefined, a) // <= ok
    return appReducer({}, a) // <= ok
    // return appReducer({ a: s.a }, a) // <= ok, partially reset the store
    // return appReducer({ a: 'a string' }, a) // <= ok BUG :( see https://github.com/facebook/flow/issues/2674
  }
  return appReducer(s, a)
}

const store: Store<State, Action> = createStore(reducer, { a: 3, b: 4 })
@aaronjensen
Contributor

Not sure why $Shape is involved, do you want to partially reset the store?

In our case, yes.

Beware: $Shape is lenient with both null and undefined

Ah, thanks.

Maybe we should exclude null explicitly

Agreed.

 // return appReducer({ a: 'a string' }, a) // <= ok BUG :( see https://github.com/facebook/flow/issues/2674

Ugh. That was already a problem though, yeah? So this wouldn't introduce a new bug afaict.

Ok, so it sounds like the | {} tweak is needed to exclude null. Other than that, I think we're good to go on this.

@gcanti
Contributor
gcanti commented Nov 22, 2016

Ugh. That was already a problem though, yeah? So this wouldn't introduce a new bug afaict.

Yes, I agree, that was already a problem. It's not critical if you use the result of combineReducers as root reducer, it's only dangerous in this use case where you call the combined reducer by hand, though I'm afraid there's nothing we can really do.

Other than that, I think we're good to go on this

I ran this change against the tests in the flow-typed repo and they are still green

@nickarora nickarora referenced this pull request in flowtype/flow-typed Nov 22, 2016
Open

define CombinedReducer type for redux_v3.x.x #475

@nickarora
Contributor
nickarora commented Nov 22, 2016 edited

@gcanti @aaronjensen updated and opened a PR in the flow-typed repo as well

@timdorr
Member
timdorr commented Nov 23, 2016

Thanks for the fix!

@timdorr timdorr merged commit c10866a into reactjs:master Nov 23, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gcanti gcanti added a commit to gcanti/pantarei that referenced this pull request Nov 24, 2016
@gcanti gcanti update redux definition (ref reactjs/redux#2115) 800486b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment