Skip to content

Wrong typings for queries by attribute #239

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

Closed
danielkcz opened this issue Dec 9, 2018 · 11 comments
Closed

Wrong typings for queries by attribute #239

danielkcz opened this issue Dec 9, 2018 · 11 comments
Labels
released TypeScript Related to TypeScript. Note: only certain maintainers handle TypeScript labeled issues.

Comments

@danielkcz
Copy link
Contributor

danielkcz commented Dec 9, 2018

  • react-testing-library version: 5.3.1
  • react version: 16.7.0-alpha.2
  • typescript version: 3.2.1
  • node version: 10.12.0
  • yarn version: 1.10.1

Relevant code or config:

const { queryByRole } = render(<Something />)
expect(queryByRole('alertdialog')).toBeFalsy()

What you did:

As you can see there is argument called text, but it's of HTMLElement type.

image

Tried the following to satisfy typings, but then it fails in runtime with error TypeError: matcher.test is not a function apparently because the container is nor string or a function, so it expects the RegExp.

image

Reproduction:

I would create one, but CodeSandbox for TypeScript is rather wonky and doesn't work correctly every time. I think that images above are a clear reproduction :) Whole repo is probably overkill

Problem description:

It's rather curious that getByRole has no such issue.

image

Suggested solution:

I tried to look into it, but my current mental capacity cannot cover this. For once I don't get how getByRole is typed correctly without any modification on react-testing-library side, but queries are wrong. This is some advanced TypeScript technique I am failing to comprehend.

@jpavon @pbomb Looks like you touched that part before. Do you have some insight perhaps?

@kentcdodds kentcdodds added the TypeScript Related to TypeScript. Note: only certain maintainers handle TypeScript labeled issues. label Dec 9, 2018
@jpavon
Copy link
Collaborator

jpavon commented Dec 9, 2018

@FredyC I just tried a clean install and works for me:

react-testing-library 5.3.1
react 16.6.3
typescript 3.2.2

screen shot 2018-12-09 at 21 15 05

@pbomb
Copy link
Collaborator

pbomb commented Dec 9, 2018

Thanks @jpavon. Does this mean that we should close this issue? Or is there still a problem?

@danielkcz
Copy link
Contributor Author

danielkcz commented Dec 9, 2018

@jpavon Thanks for looking into it. I don't get it. I just tried the separate project, all the same versions and it indeed works. In my actual project it doesn't. And it's not like VSCode would go wonky. Even when I try running TSC, I get the same...

TS2345: Argument of type '"alertdialog"' is not assignable to parameter of type 'HTMLElement'

I've even used exactly same tsconfig.json just to be sure and still it works in that simple project.

I guess I will keep digging to be able reproduce this.

@danielkcz
Copy link
Contributor Author

danielkcz commented Dec 9, 2018

Ok, I've got the repro and it's kinda caused by Yarn, but it actually behaves correctly. You see, it keeps around the [email protected] which for some reason causes the broken typings. I haven't really dug that deep. The version 3.13.1 works just fine.

https://github.com/FredyC/react-testing-library-issue-239

Even though the react-testing-library references the dependency as ^3.12.0 the Yarn won't update it simply because its lock file prevents it.

Edit: I made a pull request. As strange as it sounds, it should help since Yarn will be forced to use a new version.

@kentcdodds
Copy link
Member

What lockfile are you referring to? If it's your own, then delete the lock file and install over again

@danielkcz
Copy link
Contributor Author

I certainly don't want to delete lockfile as it would update bunch of other dependencies and possibly broke some other things. It's kinda point of a lockfile to have a consistent environment without unexpected changes.

Apparently version 3.13.1 contains some fixes that are necessary to have correct typings here. The hard dependency makes sense in that case imo.

@kentcdodds
Copy link
Member

Ah, looks like this is a yarn limitation. There's a workaround here: yarnpkg/yarn#4986 (comment)

@danielkcz
Copy link
Contributor Author

Yea sure, I can update lockfile manually, but is it really the best way? I mean it won't hurt anyone to set this hard dependency here and possible help some people that might encounter same issue in the future.

@kentcdodds
Copy link
Member

🎉 This issue has been resolved in version 5.3.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@noinkling
Copy link

noinkling commented Dec 10, 2018

@FredyC For posterity, doing a simple yarn upgrade react-testing-library (even if there isn't a new version) should update its dependencies. In this case that would just be dom-testing-library since it's the only dependency. For NPM I believe you would have to do something like npm update react-testing-library --depth=1.

If this package had more dependencies it would also try and update those, so it's less specific than the other mentioned workaround where you manually remove a piece of your lockfile, but also less hacky. It's still more specific than deleting the whole lockfile.

An update of this package is fine too though, I just popped in here to see what prompted the version bump.

@danielkcz
Copy link
Contributor Author

danielkcz commented Dec 10, 2018

@noinkling Well, hard to test that now since the latest version has a correct dependency now. Are you sure about it? The issue linked by Kent above is exactly about that. Running upgrade won't modify indirect dependencies ... yarnpkg/yarn#4986

Edit: Ok, I take this back, might be a different issue. I just tried using fixed version [email protected] in package.json and then running upgrade has actually installed the latest dom-testing-library. Interesting 🤔Not sure if it's correct behavior too though because there different typings incompatibility like that.

Personally, I haven't been using a command line for this, I usually just change numbers in package.json simply because it's more apparent what has been updated and to what version when I look in git history. It's definitely not that apparent from lock file. Also, it's super easy with VSCode Version Lens.

Either way, the point here still remains that if a dependent library introduces breaking changes (although rather subtle in this case) it should always be updated in parent packages and not to rely on version ranges only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released TypeScript Related to TypeScript. Note: only certain maintainers handle TypeScript labeled issues.
Projects
None yet
Development

No branches or pull requests

5 participants