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

Hook functions require a void return type #695

Closed
chrbala opened this issue Aug 7, 2018 · 5 comments
Closed

Hook functions require a void return type #695

chrbala opened this issue Aug 7, 2018 · 5 comments

Comments

@chrbala
Copy link

chrbala commented Aug 7, 2018

I'm not sure if this should be filed under bug or feature request.

Basically the hooks have a void return type, which means that you can't return any value, whether the return value is used or not.

type OnDragStartHook = (start: DragStart, provided: HookProvided) => void;
type OnDragUpdateHook = (update: DragUpdate, provided: HookProvided) => void;
type OnDragEndHook = (result: DropResult, provided: HookProvided) => void;

This means that we can't do:

<DragDropContext onDragStart={someFunctionThatReturnsValue} />

Instead, we have to proxy the function like this:

<DragDropContext onDragStart={(...args) => { someFunctionThatReturnsValue(...args) }} />

Since the return isn't used anyway, is there a reason why these aren't typed with a mixed return type? This is how I typically type event handlers in order to give more freedom.

@alexreardon
Copy link
Collaborator

If we change the return type to mixed it might imply that we are doing something with the return value, which we are not. I think this could be a source of confusion for people

@chrbala
Copy link
Author

chrbala commented Aug 7, 2018

This is what the flow docs recommend for cases like this. I think people familiar with flow will understand that the return value may not actually be used. If you're still concerned, you could explain why the return type is mixed by the type definition.

@alexreardon
Copy link
Collaborator

Fair enough! Happy to update the types. I think it would be also worth calling out in the docs that we do not use the return type

@alexreardon
Copy link
Collaborator

I think this would be a good candidate for a first contribution 😊

@alexreardon
Copy link
Collaborator

This will ship in our next release! Thanks @kylehalleman!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants