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

Add type tests for prepend, improve prepend type #415

Merged
merged 2 commits into from
Nov 6, 2020

Conversation

doasync
Copy link
Member

@doasync doasync commented Nov 5, 2020

prepend method infers payload type from the function argument if it exists... and if there is no argument the resulting payload type will be of type unknown. This is a bug because a user cannot call Event<unknown> without params - it waits for 1 argument with type unknown.

const event = createEvent<string>();
const prepended = event.prepend(() => 'foo') // Event<unknown>
prepended() // TS error: Expected 1 arguments, but got 0.

To bypass the type error, a user needs to specify void manually:

const event = createEvent<string>();
const prepended = event.prepend<void>(() => 'foo') // Event<void>
prepended() // ok

But we can fix this by adding a default type to prepend's Before:

  prepend<Before = void>(fn: (_: Before) => Payload): Event<Before>

As the result, TypeScript will show an error when you directly call Event<void> with an argument, which is good too:

const event = createEvent<string>()
const prepended = event.prepend(() => 'foo')
prepended(1) // Argument of type 'number' is not assignable to parameter of type 'void'.

Now it can be seen that the addition of the default value improves typing in both cases.

P.S. I also added type tests for .prepend and the described cases.

@netlify
Copy link

netlify bot commented Nov 5, 2020

Deploy request for effector-docs rejected.

Rejected with commit 5eb647f

https://docs.netlify.com/configure-builds/environment-variables/#sensitive-variable-policy

@zerobias
Copy link
Member

zerobias commented Nov 6, 2020

Cool idea! 🚀

@zerobias zerobias merged commit 6617b41 into effector:master Nov 6, 2020
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.

2 participants