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

queryByRole types aren't aligned to implementation #142

Closed
chiefGui opened this issue Oct 31, 2018 · 7 comments · Fixed by #147
Closed

queryByRole types aren't aligned to implementation #142

chiefGui opened this issue Oct 31, 2018 · 7 comments · Fixed by #147
Labels

Comments

@chiefGui
Copy link

  • dom-testing-library version: ^3.10.1
  • react version: ^16.6.0
  • node version: 9.2.0
  • npm (or yarn) version: yarn v1.10.1

Relevant code or config:

test("render tab title with icon", () => {
  expect(queryByRole(document.body, "figure")).toBeNull();

  render(
    <Tabs>
      <Tabs.Tab title="Home" icon={AlbumsIcon}>
        hello world
      </Tabs.Tab>
    </Tabs>
  );

  expect(queryByRole(document.body, "figure")).toBeTruthy();
});

What you did:

I've ran some tests.

What happened:

Tests are passing, but TypeScript is complaining.

Reproduction:

// index.test.tsx

import * as React from "react"
import { render, cleanup } from "react-testing-library"
import { queryByRole } from "dom-testing-library"
import { Something } from "."

afterEach(cleanup)

describe("something", () => {
  test("render role", () => {
    expect(queryByRole(document.body, "figure")).toBeNull()

    render(<Something />)

    expect(queryByRole(document.body, "figure")).toBeTruthy()
  })
})
// index.tsx

export const Something = () => <div role="figure" />

Problem description:

Basically, tests are passing, but TypeScript complains that queryByRole expects from 3-4 arguments and it's getting 2.

According to your typings, that seems expected though:

The first argument QueryAttribute expects is attribute: string;, but that doesn't make sense because the attribute I want is already expressed by queryByRole (which is role).

Same happens with queryByAltText. i.e.:

image

Suggested solution:

My suggested solution is to better design these interfaces to properly fit to the signatures of these methods that already have an explicit attribute. I can create a PR for that, just want to make sure if I'm not daydreaming or something.

Thanks!

@chiefGui
Copy link
Author

Also, as you can see here, queryByRole is implemented by automatically binding the role attribute. So I guess I'm not daydreaming, hah.

@JeffBaumgardt
Copy link
Contributor

I think that's my bad I did not update the typescript definitions when I added role since I'm not as versed as TS. I will look at adding that today.

@JeffBaumgardt
Copy link
Contributor

I take that back someone did add the typings from what I can tell. As seen here. I will need help from someone who knows more about TS to help troubleshoot. Perhaps @jenovs might be of assistance here.

@jenovs
Copy link
Contributor

jenovs commented Nov 1, 2018

I'm still learning the ropes of TS and my PR just removed redundant letter T from typings' names so they'll align with the function names.
And as I only used getByRole in my code I didn't notice any other problems (but I now checked queryByRole and can confirm that TS complains about missing arguments).

@chiefGui
Copy link
Author

chiefGui commented Nov 2, 2018

Well, I can create a PR this weekend to align these typing issues if you want. Just let me know your plans.

Thank you for the attentions guys!

@JeffBaumgardt
Copy link
Contributor

You are welcome to. Like I said I would help out more if I knew TS more. I just used the annotations that everyone else was using. 😄

kentcdodds pushed a commit that referenced this issue Nov 6, 2018
closes #142

<!--
Thanks for your interest in the project. Bugs filed and PRs submitted are appreciated!

Please make sure that you are familiar with and follow the Code of Conduct for
this project (found in the CODE_OF_CONDUCT.md file).

Also, please make sure you're familiar with and follow the instructions in the
contributing guidelines (found in the CONTRIBUTING.md file).

If you're new to contributing to open source projects, you might find this free
video course helpful: http://kcd.im/pull-request

Please fill out the information below to expedite the review and (hopefully)
merge of your pull request!
-->

<!-- What changes are being made? (What feature/bug is being fixed here?) -->

**What**:

<!-- Why are these changes necessary? -->

**Why**:

<!-- How were these changes implemented? -->

**How**:

<!-- Have you done all of these things?  -->

**Checklist**:

<!-- add "N/A" to the end of each line that's irrelevant to your changes -->

<!-- to check an item, place an "x" in the box like so: "- [x] Documentation" -->

- [ ] Documentation
- [ ] Tests
- [ ] Ready to be merged <!-- In your opinion, is this ready to be merged as soon as it's reviewed? -->
- [ ] Added myself to contributors table <!-- this is optional, see the contributing guidelines for instructions -->

<!-- feel free to add additional comments -->
@kentcdodds
Copy link
Member

🎉 This issue has been resolved in version 3.12.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 a pull request may close this issue.

4 participants