-
Notifications
You must be signed in to change notification settings - Fork 28
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
basic typescript definitions #18
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, I'm all for adding this to the repo, thanks! I don't have much experience with TypeScript, so excuse me if my question below is silly ^^.
connect<A, B, C>( | ||
component: (state: State, a: A, b: B, c: C) => string | ||
): (a: A, b: B, c: C) => string; | ||
connect(component: (state: State, ...args: any[]) => string): (...args: any[]) => string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this line here, are the other declarations using generics needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is sort of an unfortunate property of typescript lacking "variadic kinds", which is basically typed spread arguments.
If we only have the last signature, than any connected components will always have the function signature (...args: any[]) => string
instead of specific argument types.
For example, say we have a component like the following
const SayHi = (state: State, name: string) => html`
<div>
hey ${name}, check out this state prop ${ state.prop }
</div>
`;
and then connect it
const ConnectedSayHi = connect(SayHi);
// if we only have the last signature, this will not error,
// as the signature is (...args: any[]) => string
ConnectedSayHi(10);
// however, if we have the over loaded signature,
// then connected components with 3 or fewer arguments
// will have their arguments typechecked...
ConnectedSayHi(10) // type error, as the signature is (a: string) => string;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the current annotation basically says... If the connected function has 3 or fewer arguments, create a new function with the same arguments (minus state
), otherwise just type it as (...args: any[]) => string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation. This makes sense to me now.
}; | ||
|
||
export { createStore }; | ||
export default html; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: please add a newline at the end of the file here.
one other thought on this, might want to wait until we figure out if |
@stasm can we merge this PR now? |
Not sure if this is something you want in this lib @stasm, but these are the type definitions I used for innerself-hn and thought others might find them useful.
Alternatively we could publish them on DefinitivelyTyped, but its always nice to have them in the repo.