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

Add typescript definitions for findBy* queries #226

Closed
kentcdodds opened this issue Mar 19, 2019 · 8 comments
Closed

Add typescript definitions for findBy* queries #226

kentcdodds opened this issue Mar 19, 2019 · 8 comments

Comments

@kentcdodds
Copy link
Member

I don't trust myself with typing those, so if someone would be good enough to do that, I would appreciate it!

@alexkrolick
Copy link
Collaborator

Pretty sure it's Promise<HTMLElement | Error>, Promise<HTMLElement[] | Error>

@JaxCavalera
Copy link
Contributor

JaxCavalera commented Mar 20, 2019

I put together the type definitions for findBy and findAllBy queries, but am unable to commit due to what seems like a Windows compatibility issue:

Stashing changes... [started]
Stashing changes... [skipped]
→ No partially staged files found...
Running linters... [started]
Running tasks for README.md [started]
Running tasks for README.md [skipped]
→ No staged files match README.md
Running tasks for *.+(js|jsx|json|yml|yaml|css|less|scss|ts|tsx|md|graphql|mdx) [started]
Running tasks for *.+(js|jsx|json|yml|yaml|css|less|scss|ts|tsx|md|graphql|mdx) [failed]
→ .\node_modules\kcd-scripts\dist\index.js could not be found. Try `npm install .\node_modules\kcd-scripts\dist\index.js`.

Running linters... [failed]
→ .\node_modules\kcd-scripts\dist\index.js could not be found. Try `npm install .\node_modules\kcd-scripts\dist\index.js`.
.\node_modules\kcd-scripts\dist\index.js could not be found. Try `npm install .\node_modules\kcd-scripts\dist\index.js`.

husky > pre-commit hook failed (add --no-verify to bypass)
Completed with errors, see above.

I could bypass the pre-commit as running lint locally passes, same with npm run validate. Running npm run setup passes everything except 1 test and this turned out to be due to my machine running a little slower than the test allocated for (was fixed with the below change but left it out since it is probably just an issue on my toaster)

   container.setAttribute('data-test-attribute', 'something changed twice')
-  await skipSomeTimeForMutationObserver(50)
+  await skipSomeTimeForMutationObserver(200)

   expect(callback).toHaveBeenCalledTimes(3)

Created a PR referencing this issue, worst case it may help get the ball rolling, best case it is good to go with minor adjustments :)

@kentcdodds
Copy link
Member Author

Thanks to @JaxCavalera, this has been resolved.

@bopfer
Copy link
Contributor

bopfer commented Apr 12, 2019

This may be a naive question, but why is Error a possible return value for findBy* queries? It pretty much negates the convenience of using them over "waitForElement + getBy*". Now, I have to check to make sure the return was not Error before I can use the element. I guess I can make my own wrapper for that, but I was just curious.

@alexkrolick
Copy link
Collaborator

alexkrolick commented Apr 12, 2019

It shouldn't return an error, it returns a Promise that can reject with an error (which throws when used with async/await syntax)

const btn = await findByText('add user')
expect(await findByText('home')).toBeInTheDocument()

@bopfer
Copy link
Contributor

bopfer commented Apr 12, 2019

@alexkrolick, Hmm, maybe there is a discrepancy. What you typed in the 2nd comment in this issue is not what is in the typings. In the typings it is Promise<HTMLElement> | Error. Which means for TypeScript to be happy in my tests I have to do:

const btn = await findByText('add user');
if (btn instanceof HTMLElement) {
  // do the rest of the test
}
else {
  throw new Error(btn.message);
}

@alexkrolick
Copy link
Collaborator

If you're a Typescript user, care to make a PR to fix that?

@bopfer
Copy link
Contributor

bopfer commented Apr 12, 2019

Will do. I think the | Error part should just be removed completely. Since the error will be thrown, an Error will never actually be returned.

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

No branches or pull requests

4 participants