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

[Typings] improve userEvent.type typings to only return a Promise when using delay is set #480

Closed
gndelia opened this issue Nov 3, 2020 · 6 comments · Fixed by #491
Closed
Labels
enhancement New feature or request

Comments

@gndelia
Copy link
Collaborator

gndelia commented Nov 3, 2020

userEvent.type always returns a promise, but we only need to await it if we're using the { delay: number } as the third argument, according to the docs

// this returns a Promise, although it is not used
userEvent.type(getByRole('button'))

In js that's fine, but in TS, the compiler or eslint might warn you about having floating promises.

that could force the TS users to always use await even though it might not be necessary - it's different from js where we could just not await it.

A possible solution could be to use type overloading when defining the function type, so it returns Promise<void> if { delay: number } is passed, but no value is provided, return void

I think that could be achieved with the following code

type TypeWithoutDelay = (element: TargetElement, text: string, userOpts?: Omit<ITypeOpts, 'delay'> ) => void
type TypeWithDelay = (element: TargetElement, text: string, userOpts: Omit<ITypeOpts, 'delay'> & { delay: number }) => Promise<void>
// then on userEvent.type
declare const userEvent: {
  //...
  type: TypeWithDelay & TypeWithoutDelay
  //...
}

That should allow the inference to work

skipping options
image

not passing delay
image
image

using delay: number (which gives us the Promise as expected)
image
image

While this works, I wanted to read your thoughts as the code does return a Promise (instead of void), just that it is not required to be awaited, so it could be misleading, or perhaps it's not worth the effort. I could do the PR as it is just as simple as updating the typings with that option.

@ljosberinn
Copy link
Contributor

@gndelia
Copy link
Collaborator Author

gndelia commented Nov 3, 2020

oh lol delay is a number, not a boolean. My bad 😆 I guess it still applies the same idea

edit: I updated the PR details and title to reflect it is a number and not a boolean

@gndelia gndelia changed the title [Typings] improve userEvent.type typings to only return a Promise when using "delay: true" [Typings] improve userEvent.type typings to only return a Promise when using delay is set Nov 3, 2020
@nickmccurdy
Copy link
Member

nickmccurdy commented Nov 11, 2020

@laquasicinque As clever as extends {delay: 0} is, it's unfortunately not very practical. Most users will be used to {delay: 0}, which is widened to the type {delay: number}, which will fail this check and return a Promise, which leads to the original issue here. While users could use {delay: 0 as const}, it's not very easy to learn, and I think going with a slightly more naive type is better than a confusing error message. Thanks for the idea though.

@nickmccurdy
Copy link
Member

I have committed @ljosberinn's types from #480 (comment) and added tests in #491.

@nickmccurdy nickmccurdy linked a pull request Nov 11, 2020 that will close this issue
4 tasks
@nickmccurdy nickmccurdy added the enhancement New feature or request label Nov 11, 2020
@gndelia
Copy link
Collaborator Author

gndelia commented Nov 11, 2020

Thanks, it looks great! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants