chore: Migrate `ByRole` to TypeScript #1186

DaniAcu · 2022-10-28T13:17:31Z

What:
Migrate role.js to role.ts.

Why:

Because is the only file that is in js. I wold like to add more inteligence to ByRole, but for any other thing we need to add it first as ts file.

How:

Checklist:

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

codesandbox-ci · 2022-10-28T13:18:34Z

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 9e5d4b4:

Sandbox	Source
react-testing-library-examples	Configuration

eps1lon

For TS migration please don't change runtime behavior (yet). This makes reviewing harder than it needs to be.

If the type-checker has issues with things, we can use @ts-expect-error or as any for now and then slowly migrate to a more type-safe version.

DaniAcu · 2022-10-29T09:03:25Z

@eps1lon Ohh okay, thanks. I didn't know that but that make sense. Let me update the PR to just have ts migration here. Thanks, again

eps1lon

Lint is failing. Is there anything else that's unclear to you from the CI logs?

CI needs to be green for this to be mergeable. If you're stuck at some point, feel free to ping me and we can go through the CI logs to see what went wrong.

codecov · 2022-10-31T23:57:50Z

Codecov Report

Merging #1186 (d72f79a) into alpha (25dc8a9) will not change coverage.
The diff coverage is 100.00%.

❗ Current head d72f79a differs from pull request most recent head 9e5d4b4. Consider uploading reports for the commit 9e5d4b4 to get more accurate results

@@            Coverage Diff            @@
##             alpha     #1186   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           24        24           
  Lines          998      1000    +2     
  Branches       327       328    +1     
=========================================
+ Hits           998      1000    +2

Flag	Coverage Δ
node-12	`100.00% <100.00%> (?)`
node-14	`100.00% <100.00%> (ø)`
node-16	`100.00% <100.00%> (ø)`
node-18	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/query-helpers.ts	`100.00% <ø> (ø)`
src/queries/role.ts	`100.00% <100.00%> (ø)`
src/helpers.ts	`100.00% <0.00%> (ø)`
src/queries/label-text.ts	`100.00% <0.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

DaniAcu · 2022-11-01T00:08:56Z

@eps1lon I delayed more in make the fixes becuase lint is not working locally.

So, I used the github jobs to check and fix it.

Related to the changes, I think that the types are not completly clean or organized. I will try to get into it a little bit more now that roles are also in ts.

Thanks for awesome welcoming to contribute here, I'm happy to could do it.

DaniAcu · 2022-11-03T12:43:45Z

@eps1lon there anything more that i need to change? 🤔

DaniAcu · 2023-01-10T02:58:01Z

@eps1lon I updated based on main and fix a error after merge. Could you re check it?

eps1lon

Sorry for letting this slide. This is going in the right direction.

eps1lon · 2023-01-10T07:33:14Z

src/queries/role.ts

+  // guard against using `level` option with any role other than `heading`
+  if (level !== undefined && role !== 'heading') {
+    throw new Error(`Role "${role}" cannot have "level" property.`)


The double nesting is intentional to be consistent with the other option. It allows folding away options in your code editor you're not interested in. Please revert this change.

Ohh! It's to be cosisten with the other queries right? Sounds good.

eps1lon · 2023-01-10T07:34:51Z

src/queries/role.ts

-        const roleValue = node.getAttribute('role')
+        /* istanbul ignore next */
+        const roleValue = node.getAttribute('role') ?? ''


Let's use a non-nullable assertion instead. Generally, let's not change any runtime in these migrations. We can do them in a follow-up.

We don't need the fallback since roleValue will always be defined since we just confirmed that node.hasAttribute('role')

eps1lon · 2023-01-10T07:35:24Z

src/queries/role.ts

@@ -129,7 +156,7 @@ function queryAllByRole(
        return matcher(firstWord, node, role, matchNormalizer)
      }

-      const implicitRoles = getImplicitAriaRoles(node)
+      const implicitRoles = getImplicitAriaRoles(node) as string[]


question: what would the type be without the cast`

role-helpers need migration to typescript so it return any[]

eps1lon · 2023-01-10T08:12:11Z

src/queries/role.ts

-  Array.from(container.children).forEach(childElement => {
+  Array.from(
+    /* istanbul ignore next */
+    container?.children ?? [],


Same here. Let's not change runtime and use type casting/assertions instead.

eps1lon · 2023-01-10T08:13:27Z

types/__tests__/type-tests.ts

@@ -178,15 +178,9 @@ export async function testByRole() {
  console.assert(queryByRole(element, 'button', {name: /^Log/}) === null)
  console.assert(
    queryByRole(element, 'button', {
-      name: (name, el) => name === 'Login' && el.hasAttribute('disabled'),
+      name: (name, el) => name === 'Login' && !!el?.hasAttribute('disabled'),


why does this need to change?

It's also pretty hard to grok what this is doing.

Name require a boolean value. el variable could be undefined based on typing. The boolean casting is because type result could be udefined by el or boolean by hasAttribute

Maybe is more readable like that

name: (name, el) => name === 'Login' && !!el && el.hasAttribute('disabled'),

Why is el nullable? Shouldn't that always be defined?

eps1lon · 2023-01-10T08:13:38Z

types/__tests__/type-tests.ts

-  console.assert(queryByRole(element, 'foo') === null)
-  console.assert(queryByRole(element, /foo/) === null)
-  console.assert(screen.queryByRole('foo') === null)
-  console.assert(screen.queryByRole(/foo/) === null)


Why remove these?

Because with the right typing it not be allowed to do it. If we want to continue using it, we could skip typescript in ths couple of lines

Why is this not allowed? query* can return a nullable value

Because "foo" is not a valid role according to types

eps1lon · 2023-01-10T08:15:11Z

types/queries.d.ts

-    | RegExp
-    | string
-    | ((accessibleName: string, element: Element) => boolean)
+  name?: RegExp | string | MatcherFunction


The original signature included useful type hints (e.g. accessibleName). These are lost with a generic MatcherFunction.

Could we instead use a concrete NameMatcherFunction to get type hints and keep the improved union?

eps1lon · 2023-01-10T08:15:47Z

types/queries.d.ts

-  role: ByRoleMatcher,
-  options?: ByRoleOptions,
-) => T[]
+export type AllByRole = QueryMethod<


Same here. By using the generic QueryMethod we loose type hints. Let's type it out properly.

Oh okay, I think QueryMethod was the right way. Do you know what are the type hints that we lose that you mention.

Just to take in care when I work in that

In editor suggestions:
original:

your change:

Notice the name of the parameters

DaniAcu · 2023-01-16T02:54:07Z

@eps1lon hi, i keep the changes simple and i dismiss any change that affect run time. There some ts comments, but I think it's fine for the migration. There others things that need to be change in general, but for the proposal of this PR, i think that with this changes is enough.

Let me know if there any thing that you look weird in this changes.

And thanks in advance for the time that you take to revisit it

eps1lon

Added some finishing touches. Thank you for sticking with it!

eps1lon · 2023-02-06T18:54:38Z

@all-contributors add @DaniAcu for code

allcontributors · 2023-02-06T18:54:48Z

@eps1lon

I've put up a pull request to add @DaniAcu! 🎉

eps1lon requested changes Oct 28, 2022

View reviewed changes

eps1lon requested changes Oct 31, 2022

View reviewed changes

DaniAcu requested a review from eps1lon November 1, 2022 01:00

eps1lon requested changes Jan 10, 2023

View reviewed changes

DaniAcu requested a review from eps1lon January 16, 2023 02:54

eps1lon changed the base branch from main to alpha February 6, 2023 17:55

DaniAcu added 17 commits February 6, 2023 18:56

fix: migrate role to ts

4fc28e5

fix: remove role model abstraction

36d53db

fix: prettier

b7e403c

fix: force have role that are included in type

3b24e00

fix: type

037c073

fix: error and add commet ts

98568cd

fix: any type

984acb9

fix: any type

9fab0b8

fix: type assertion

82503fb

fix: ignore branch coverage

3de4aa9

fix: typo

a1fb8c1

fix: add hints in all by role

874e769

fix: types and comments

7431f75

fix: restore types

8a3c1a0

fix: restore types

df14122

fix: lint

4423a51

fix: ts error

4de62b3

DaniAcu and others added 4 commits February 6, 2023 18:57

fix: ts error

1ea74bd

fix: type tests

8ae5a9c

Fix lint

94163c1

Don't couple query-helpers with ByRole

30d44d0

eps1lon force-pushed the fix/migrate-role-to-ts branch from d72f79a to c88c915 Compare February 6, 2023 18:15

Revert change to ByRoleMatcher type

9e5d4b4

eps1lon force-pushed the fix/migrate-role-to-ts branch from c88c915 to 9e5d4b4 Compare February 6, 2023 18:19

eps1lon changed the title ~~fix: migrate role to ts~~ chore: migrate role to ts Feb 6, 2023

eps1lon approved these changes Feb 6, 2023

View reviewed changes

eps1lon changed the title ~~chore: migrate role to ts~~ chore: Migrate ByRole to TypeScript Feb 6, 2023

eps1lon merged commit 42809fe into testing-library:alpha Feb 6, 2023

allcontributors bot mentioned this pull request Feb 6, 2023

docs: add DaniAcu as a contributor for code #1212

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Migrate `ByRole` to TypeScript #1186

chore: Migrate `ByRole` to TypeScript #1186

DaniAcu commented Oct 28, 2022 •

edited by eps1lon

Loading

codesandbox-ci bot commented Oct 28, 2022 •

edited

Loading

eps1lon left a comment

DaniAcu commented Oct 29, 2022

eps1lon left a comment

codecov bot commented Oct 31, 2022 •

edited

Loading

DaniAcu commented Nov 1, 2022

DaniAcu commented Nov 3, 2022

DaniAcu commented Jan 10, 2023

eps1lon left a comment

eps1lon Jan 10, 2023

DaniAcu Jan 10, 2023

eps1lon Jan 10, 2023

eps1lon Jan 10, 2023

DaniAcu Jan 11, 2023

eps1lon Jan 10, 2023

eps1lon Jan 10, 2023

DaniAcu Jan 10, 2023

eps1lon Jan 10, 2023

eps1lon Jan 10, 2023

DaniAcu Jan 10, 2023

eps1lon Jan 10, 2023

DaniAcu Jan 10, 2023

eps1lon Jan 10, 2023

eps1lon Jan 10, 2023

DaniAcu Jan 10, 2023

DaniAcu Jan 10, 2023

eps1lon Jan 10, 2023

DaniAcu Jan 10, 2023

DaniAcu Jan 10, 2023

DaniAcu commented Jan 16, 2023

eps1lon left a comment

eps1lon commented Feb 6, 2023

allcontributors bot commented Feb 6, 2023

chore: Migrate ByRole to TypeScript #1186

chore: Migrate ByRole to TypeScript #1186

Conversation

DaniAcu commented Oct 28, 2022 • edited by eps1lon Loading

codesandbox-ci bot commented Oct 28, 2022 • edited Loading

eps1lon left a comment

Choose a reason for hiding this comment

DaniAcu commented Oct 29, 2022

eps1lon left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 31, 2022 • edited Loading

Codecov Report

DaniAcu commented Nov 1, 2022

DaniAcu commented Nov 3, 2022

DaniAcu commented Jan 10, 2023

eps1lon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaniAcu commented Jan 16, 2023

eps1lon left a comment

Choose a reason for hiding this comment

eps1lon commented Feb 6, 2023

allcontributors bot commented Feb 6, 2023

chore: Migrate `ByRole` to TypeScript #1186

chore: Migrate `ByRole` to TypeScript #1186

DaniAcu commented Oct 28, 2022 •

edited by eps1lon

Loading

codesandbox-ci bot commented Oct 28, 2022 •

edited

Loading

codecov bot commented Oct 31, 2022 •

edited

Loading