-
Notifications
You must be signed in to change notification settings - Fork 670
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
TypeScript typings for reselect? #55
Comments
I would be very happy if someone decided to create Typescript definitions. We did make some changes for the 1.0.0 release that should make them possible (#27). |
It would also be interesting to see Flow types as well. I'm not sure whether the state of Flow is far enough along to cover all the ES6 we use. I wonder whether it's possible to generate one from the other. |
Need this as well, right now I'm debugging errors since 1/2 hour in my code due to incorrect manual types I added when composing selectors. If the compiler would know then it would be a ton of help (plus would reduce a lot of the manual effort needed to propagate the types). I would like to help exept that I'm pretty sure I don't know enough about the typing system to write the types definitions. |
for myself i created just most stupid definition:
|
How about:
|
@frankwallis , I just added your type definition to a project with some 40+ selectors. It seems to work pretty well. For anyone who don't like to type a lot, here's some more of the "etc" part:
|
Yes they're working pretty well for me too, and providing type-inference in the 'combiner' function which is really useful. Thanks for filling out more of the overloads, I've submitted a PR to DefinitelyTyped. |
@frankwallis works nicely, thanks a bunch! |
Hey, this is great, thanks everyone! I'll add a link to here from the readme. Do you think you could let me know if they accept the PR to DefinitelyTyped please? I'll update the readme to point there instead if that happens. |
Ok will do - I'm not sure on the best way to get it merged but it may help if the PR has been 'approved' by a member of the reselect project... Until it is merged if you are using tsd you can manually put the PR commit hash in tsd.json to install this declaration file:
|
That PR has now been merged. |
Thanks |
Would you guys be willing to include the typings in the NPM package? The main benefit being that the typings can be managed directly in this repo, and users do not have to go to a third-party source to get typings for this lib. I would be more than willing to implement this. https://github.com/Microsoft/TypeScript/wiki/Typings-for-npm-packages |
@ianks Yes, please do! |
After having used the typing for a while, I noticed it isn't playing well with Since the So I'm experimenting with this instead: type Selector<TInput, TProps, TOutput> = (state: TInput, props?: TProps) => TOutput;
function createSelector<TInput, TProps, TOutput, T1>(selector1: Selector<TInput, TProps, T1>, combiner: (arg1: T1) => TOutput): Selector<TInput, TProps, TOutput>;
function createSelector<TInput, TProps, TOutput, T1, T2>(selector1: Selector<TInput, TProps, T1>, selector2: Selector<TInput, TProps, T2>, combiner: (arg1: T1, arg2: T2) => TOutput): Selector<TInput, TProps, TOutput>; Seems to work fine. Any comments? |
Thanks for the suggestion. I don't know Typescript well enough to be confident in commenting. @ianks @frankwallis @geon: do any of you want commit access to Reselect to take care of Typescript issues? |
@geon that looks good. can you create a minimal example case for this (just to show that that types are retained from connect)? |
Here's a semi-realistic example of what our code looks like: delete_button_component.ts export type DeleteButtonProps = DeleteButtonStateProps & DeleteButtonDispatchProps;
export type DeleteButtonStateProps = {
disabled: boolean
}
export type DeleteButtonDispatchProps = {
onClick: event => void
}
export function deleteButton({
disabled,
onClick
}: DeleteButtonProps) {
return DOM.button({
disabled,
onClick
}, 'Delete');
} delete_button_container.ts type DeleteButtonContainerProps = {
itemId: string
}
const mapStateToProps = createSelector<
// NOTE! 3 types for the input/output instead of 2 as before.
RootState, DeleteButtonContainerProps, DeleteButtonStateProps,
boolean
> (
(state, props) => !!state.items[props.itemId],
(itemExists: boolean) => ({
disabled: !itemExists
})
);
function mapDispatchToProps (dispatch: ReactRedux.Dispatch, props: DeleteButtonContainerProps): DeleteButtonDispatchProps {
return {
onClick: event => dispatch(actions.deleteItem(props.itemId))
};
}
export const DeleteButton = connect(mapStateToProps, mapDispatchToProps)(deleteButton); Now, calling the container with anything else than Using it: function render () {
return DOM.div({}
SomeItemView({
foo: 'foo',
bar: 'bar',
itemId: theItemId
}),
DeleteButton({
itemId: theItemId
})
);
} We don't use jsx, so I don't really know how to write a more idiomatic example. Seems like nobody else use the DOM api manually. :P This code has obviously never run, so I'm sure there are some bugs in it. But I think you get the gist. Without the props typing I suggested, |
@ellbee , I'm not sure I should have commit access. I still feel like a react noob. I've only been using it since christmas. |
I need to just be sure that it does not become necessary to specify the type parameters when calling
So I specify the shape of the global state and the inputs to the functions and then get type inference in the 'combiner' function and the resulting selectors. The type inference feeds into other selectors when you start combining them. I would be quite unhappy if I had to start specifying all the type parameters whenever I called createSelector, so I would like to check that first. Can I also ask if you are using |
Yeah. For some reason, we didn't get that to work earlier. Possible because we had bad typing in other places. (We migrated to ts with a lot of js already written.) I should try again.
We only use it as a function, since 99% of our components are stateless functions, not classes. I suppose there really should be a few test cases to ensure the typings work in all the intended situations. |
I tried using Modified example from above (that actually compiles, yay!): const mapStateToProps = Reselect.createSelector(
(state: RootState, props: DeleteButtonContainerProps) => !!state.items[props.itemId],
(itemExists: boolean) => ({
disabled: !itemExists
})
); I suppose it would be impossible to not type it explicitly somewhere. In our case, we have a lot ot "primitive" selectors, so it is nice to be able to type them in |
So I have given this a try, I did get some compiler errors because I am using the
Unfortunately it is not possible to specify default values for generic parameters, but hopefully that will be possible soon (see here). If it were possible it sounds like they would have to be at the end of the parameter list so I think that the TProps param should be moved to the end. I have this working and backwards compatible as follows:
When default generic parameters are available it will be possible to unify If people are already calling
|
I added tests and props typing in a pull-request. Please review: #115 |
For some reason, i have the typescript compiler complaining that the reselect module is unknown. import {createSelector} from 'reselect'; |
@vjau Can you open a new issue with information on TypeScript version and contents of tsconfig.json? TS2307 can happen for hundreds of different reasons. |
I had this error when "module" in my tsconfig.json was set "es6", try importing using
|
@screenpunch you do not get the typed version if you load it that way. |
To everyone interested, please help review updated typings: #192 |
Do you provide TypeScript typings for reselect?
The text was updated successfully, but these errors were encountered: