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

fix: explicitly mention Matcher type in BoundFunction TS type #335

Closed
wants to merge 1 commit into from
Closed

fix: explicitly mention Matcher type in BoundFunction TS type #335

wants to merge 1 commit into from

Conversation

hmttrp
Copy link

@hmttrp hmttrp commented Aug 6, 2019

What:

Changing typescript type definitions for BoundFunction to not infer the type of the text property anymore but explicitly reference the type Matcher instead.

Why:

Using testcafe-testing-library along with typescripts creates a typing error.

This seems to be due to wrongly inferred types.

Screenshot 2019-08-06 at 16 30 16

Screenshot 2019-08-06 at 16 29 52

In the image above you can see, that the text property should be of type SelectorMatcherOptions | undefined and the options type is unknown which is both wrong.

It should be

(text: Matcher, options?: SelectorMatcherOptions | undefined) => Selector

I think this is a regression from #139

How:

Removing type inference and use Matcher type explicitly. I did not find a case where text is not of type Matcher.

Checklist:

  • Documentation added to the
    docs site
  • Typescript definitions updated
  • Tests
  • Ready to be merged

Additional Comment
I tried to validate the types by using npm run dtslint but it fails for typescript 2.8. Also the master branch is failing for me. Can someone validate if this is true or if it is an issue on my machine.

Type inference was not working properly in some cases. Since all queries
have the `Matcher` type for the text property the types can be explicit
here.
Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to me, but I don't do much with the TypeScript so I'd like another reviewer please. Thanks!

@kentcdodds
Copy link
Member

npm run dtslint is failing on master for me as well.

@hmttrp
Copy link
Author

hmttrp commented Aug 6, 2019

I just recognised, that some queries also do have a waitForElementOptions property.
This is currently not reflected by the typescript type. Should this be added?
And is there a good way to check if this still works with the other libraries since this might have an impact?

@kentcdodds
Copy link
Member

There's not a way to check AFAIK. Maybe npm link can help?

We definitely want the typings to be as accurate as possible.

@hmttrp
Copy link
Author

hmttrp commented Aug 7, 2019

One way I thought that would additionally support the waitForElementOptions could be.

export type BoundFunction<T> = 
    T extends (attribute: string, element: HTMLElement, ...rest: infer P) => infer R 
    ? (...rest: P) => R
    : T extends (element: HTMLElement, ...rest: infer P) => infer R 
        ? (...rest: P) => R
        : never

But it does not seem to work.
Anyone with typescript knowledge has an opinion on this?

@kentcdodds
Copy link
Member

I honestly have no idea. Unfortunately we don't really have any consistent maintainers of the TypeScript definitions and to be frank I'm getting tired of drive-by contributors making changes to the definitions which I can't really verify 😬

If you're willing to commit to being a dedicated TypeScript maintainer then that's great. Otherwise I'm starting to think we should just remove the TypeScript definitions and have people put it in DefinitelyTyped instead ☹

@hmttrp
Copy link
Author

hmttrp commented Aug 13, 2019

Fair point.

I am just starting to use typescript in production application and therefore don't think that it would be reasonable to maintain the types for a bigger project.

Will close this in favor of #337

@hmttrp hmttrp closed this Aug 13, 2019
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