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

Mapped type for combineReducers in index.d.ts #2177

Closed
wants to merge 2 commits into from
Closed

Mapped type for combineReducers in index.d.ts #2177

wants to merge 2 commits into from

Conversation

mkusher
Copy link
Contributor

@mkusher mkusher commented Jan 1, 2017

Since typescript 2.1 there is a cool feature microsoft/TypeScript#12114 which can improve type inference for combineReducers. No need to explicitly pass state type to combineReducers any more.

Note: it requires typescript 2.1

Updated typescript to 2.1.4
Updated typescript-definition-tester to 0.0.5
Updated typescript tests to use proper import
Added mapped type to index.d.ts
@kevinthiart
Copy link

kevinthiart commented Jan 2, 2017

This doesn't only allow for more convenient type inference: Assuming your Reducers are defined properly, this will also enforce, at compile time, most (or all?) of the validation done in combineReducers, such as "No reducer provided for key", "Reducer returned undefined during initialization", "Reducer returned undefined when probed with a random type", etc.

It will also solve an issue where Reducer<any> can be satisfied with something like:

(state?: SomeState, action: Action) => NotSomeState;

In other words, a function that returns NotSomeState instead of SomeState. For this reason alone I'd love for all instances of Reducer<any> to be removed from the Redux typedefs.

This does make the typedef much more strict though. It is a breaking change for people who ignored the above warnings, or people who use the import * as reducers from './reducers' trick, which could create an object with unexpected properties, some of which aren't reducers.

@mkusher
Copy link
Contributor Author

mkusher commented Jan 2, 2017

@kevinthiart you're right, but ts 2.1 is also a breaking change so. Btw, I don't see any other places where Reducer<any> type is used

@aikoven
Copy link
Collaborator

aikoven commented Jan 3, 2017

Since this is a breaking change, shouldn't this be rebased onto the next branch? Not really sure though because next is very outdated. See #2165. Also #1342

Let's also update Reducer type to enable strict null checking:

export type Reducer<S> = <A extends Action>(state: S | undefined, action: A) => S;

Updated Reducer<S> type in index.d.ts
Add strictNullChecks flag to typescript spec
@mkusher
Copy link
Contributor Author

mkusher commented Jan 3, 2017

@aikoven I've added strict null check to spec & undefined to reducer. Not sure what needs to be done with the next branch, should I resend PR with target branch next? As I see there is a difference between index.d.ts in master and in next branch. So should I include only my changes or whole index.d.ts?

@aikoven
Copy link
Collaborator

aikoven commented Jan 5, 2017

cc. @timdorr

@timdorr
Copy link
Member

timdorr commented Jan 5, 2017

@mkusher I'm not sure of how this would be handled according to semver, but I think it would be a bad idea to break typing compatibility on a patch or minor version bump. The conservative approach would be to wait until the next major, which is what the next branch is for.

I would see about getting these typings published under the @types npm namespace too. Those can be versioned independently of the library, to correspond better with TS features.

@mkusher
Copy link
Contributor Author

mkusher commented Jan 5, 2017

Closing this PR in favor of #2182

@mkusher mkusher closed this Jan 5, 2017
@mkusher mkusher deleted the combine-reducers-type branch January 5, 2017 20:02
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.

4 participants