-
Notifications
You must be signed in to change notification settings - Fork 125
feat: improve typedefs #41
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
Conversation
Hi @vsgoulart! Thanks for your PR and sorry for not getting back earlier. The way I understand it these type definitions do make sense if each event contains the same payload. This, however is something we cannot guaranee. I.e. a Is there any generic patter that supports more general payloads? |
Thanks for getting back @nikku export default class FormCore {
// ...
on(event: 'submit' | 'changed', callback: (state: { data: Data; errors: Errors; }) => void): void;
on(event: 'destroy', callback: () => void): void;
// ...
} But it looks like the TS compiler can't properly infer the types from JSDoc yet (I tried some of the examples of the alternative solutions there, but they didn't really help). Considering that I have 2 suggestions. Option 1: /**
* @type { (event: Events, callback: (state: EventPayload | undefined /* | any other type we might have */) => void) => void }
*/ The problem with this solution is that we can't match the event with the callback type, so the user would have to check the callback params. For example: // Considering this is the annotation on JSDoc
// @type { (event: Events, callback: (state: EventPayload | undefined ) => void) => void }
// The user would have to do this
form.on('submit', (callbackOptions) => {
if (callbackOptions === undefined) {
return;
}
const {errors, data} = callbackOptions;
}); Option 2: /**
* @type { (event: Events, callback: (state: any) => void) => void }
*/ This is one is slightly better than the types we have on Of course there's always the option to manually create the types for TS, but it's not convenient and more prone to bugs. |
Sorry for the long wait on a response, this issue somehow fell of my radar. I'd personally go for your option 2 right now.
If that serves as an improvement for you could you update your contribution accordingly? |
9ece660
to
94194c2
Compare
@nikku Updated |
No description provided.