Skip to content

fix(types): allow bound function to accept string and regex matcher #683

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

Merged
merged 6 commits into from
Jul 17, 2020

Conversation

timdeschryver
Copy link
Member

@timdeschryver timdeschryver commented Jul 7, 2020

What:

Why:

How:

  • Removed the method overload on the getByRole queries
  • Creating a ByRoleMatcher that uses the String type instead of the string type we can have intellisense while not introducing a breaking change (we are still able to use a string that isn't included in the AriaRole union).
  • In 3938e92 I tried to explicit type the bound functions, but this also introduced a breaking change to several parts of the API.

Checklist:

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

I think for now, using String is the best solution. We can provide intellisense while not introducing a breaking change. We could revisit the types when we release a new major version.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 7, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 5d628fa:

Sandbox Source
kentcdodds/react-testing-library-examples Configuration

@codecov
Copy link

codecov bot commented Jul 7, 2020

Codecov Report

Merging #683 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #683   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           24        24           
  Lines          614       614           
  Branches       156       156           
=========================================
  Hits           614       614           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd0d670...5d628fa. Read the comment docs.

// resulting in no intellisense
// We still want to support roles not included in the union type
/* tslint:disable:ban-types */
export type ByRoleMatcher = ARIARole | String | RegExp | MatcherFunction
Copy link
Member

Choose a reason for hiding this comment

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

This use case is tracked in microsoft/TypeScript#29729.

I'd prefer we go with the approach that is documented in that issue: 'literal' | (string & {}) (microsoft/TypeScript#29729 (comment)). Then we at least follow community best practices. And we should probably link the issue in the comment in case it gets resolved.

Copy link
Member Author

Choose a reason for hiding this comment

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

🤯 I was looking for a workaround but I couldn't find any.
Thanks for also giving this a look @eps1lon , this is simply great!

@benmonro
Copy link
Member

@timdeschryver looks 7.20.0 also broke @testing-library/testcafe as well with the following failure:

FAIL ./selectors.test.ts
[test:unit:monorepo]   ● Test suite failed to run
[test:unit:monorepo] 
[test:unit:monorepo]     ../../src/index.ts:71:14 - error TS2349: This expression is not callable.
[test:unit:monorepo]       Each member of the union type '{ (container: HTMLElement, role: Matcher, options?: ByRoleOptions | undefined): HTMLElement[]; (container: HTMLElement, role: ARIARole, options?: ByRoleOptions | undefined): HTMLElement[]; } | ... 13 more ... | FindAllByBoundAttribute' has signatures, but none of those signatures are compatible with each other.
[test:unit:monorepo] 
[test:unit:monorepo]     71       return window.TestingLibraryDom[query](
[test:unit:monorepo]                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[test:unit:monorepo] 
[test:unit:monorepo] Test Suites: 1 failed, 1 total

Here's the link w/ all the details: https://github.com/testing-library/testcafe-testing-library/pull/204/checks?check_run_id=830176994

seems related, but maybe i'm wrong...

@timdeschryver
Copy link
Member Author

@benmonro this fix should also resolve that issue.

@timdeschryver
Copy link
Member Author

I have rebased this branch, should be good to go for another review.

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.

This looks fine to me, but I'm not super experienced with TypeScript and would prefer someone else give this the 👍

@kentcdodds
Copy link
Member

Anyone with rights can feel free to merge.

@benmonro benmonro merged commit 62e5eb3 into master Jul 17, 2020
@benmonro benmonro deleted the pr/680 branch July 17, 2020 14:36
@kentcdodds
Copy link
Member

🎉 This PR is included in version 7.21.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

Signature for BoundFunction<getByRole> does not accept RegExp in 7.20.0
4 participants