fix(TS): Forbid async function as callback argument for waitFor

[Lukas-Kullmann]

What:

Change typescript definitions to forbid async functions in waitFor.

Why:

Promises returned by waitFor do not work as one might expect (i.e. a rejected promise does not mean the same thing as a throw in the function). So these examples work differently:

await waitFor(() => throw new Error());
await waitFor(async () => throw new Error());

So in order to prevent a user from using an async function, a typescript error would be good. This is what this PR adds.

How:

I've added a conditional type to the return value of waitFor's callback that makes it impossible to return a promise from the callback.

Checklist:

• Documentation added to the
  docs site
• I've prepared a PR for types targeting
  DefinitelyTyped
• Tests
• Ready to be merged
  

I did not check the first three points because

• I'm not sure if it should be explicitly stated in the documentation that a promise returned in the callback might lead to unexpected behavior. But if so, this is not directly related to this PR.
• I don't know where the types are maintained. They are here, but also over at DefinitelyTyped's. But it also does not seem that they are automatically transferred. I also did not find an information about that in the contributing.md file.
• I would love to add a test, but the pre-commit hook that builds typescript files does not allow it. With dtslint, you can add an $ExpectError comment to test error cases. But if I add such a test to the type-tests.ts file, the build in the pre-commit hook fails. And I did not find a solution to disable the build for that file.

Also, I did not find an information how you correctly prefix the commit message. So I hope I did it correctly. :)

[codesandbox-ci]

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 839be13:

Sandbox	Source
lively-voice-d1di0	Configuration

[codecov]

Codecov Report

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

@@            Coverage Diff            @@
##            master      #572   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           23        23           
  Lines          462       462           
  Branches       114       114           
=========================================
  Hits           462       462           

---

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 b12eef1...839be13. Read the comment docs.

[eps1lon]

I would love to add a test, but the pre-commit hook that builds typescript files does not allow it. With dtslint, you can add an $ExpectError comment to test error cases. But if I add such a test to the type-tests.ts file, the build in the pre-commit hook fails. And I did not find a solution to disable the build for that file

I have an idea for that.

In the meantime you could use https://www.typescriptlang.org/play/ to illustrate how you'd test that.

[eps1lon]

Just checked and it definitely works. Either your test or implementation is faulty. Just push the test so that we can take a look at it together.

[Lukas-Kullmann]

I cannot commit it. :D

Basically what I want to do is to add this to the test file:

  await waitForElementToBeRemoved(getByText(element, 'apple'))
  await waitForElementToBeRemoved(getAllByText(element, 'apple'))
+
+  // $ExpectError
+  await waitFor(async () => {})
}

But if I want to commit it, the commit fails:

> git commit -m "Add test for passing async function to waitFor"
husky > pre-commit (node v12.16.3)
✔ Preparing...
⚠ Running tasks...
  ↓ No staged files match README.md [SKIPPED]
  ✔ Running tasks for *.+(js|jsx|json|yml|yaml|css|less|scss|ts|tsx|md|graphql|mdx|vue)
  ❯ Running tasks for *.+(ts|tsx)
    ✖ tsc --noEmit [FAILED]
↓ Skipped because of errors from tasks. [SKIPPED]
✔ Reverting to original state because of errors...
✔ Cleaning up... 

✖ tsc --noEmit:
types/__tests__/type-tests.ts(124,17): error TS2345: Argument of type '() => Promise<void>' is not assignable to parameter of type '() => never'.
  Type 'Promise<void>' is not assignable to type 'never'.
husky > pre-commit hook failed (add --no-verify to bypass)

Which is expected. We want to have an error there. I think the best way would be to not build the ts files here. The test of the types is done by dtslint.
The issue is that this is defined in the kcd-scripts. And I see why it wants to build ts files there. In a generic project, it would make sense to build ts files. But if it's tests for typings, not so much.

Here's a playground link: https://www.typescriptlang.org/play/#code/CYUwxgNghgTiAEAzArgOzAFwJYHtXwHcosMAxHGAHgBUA+ACjCgggCMowBrALnnoEp4AXlrxq8EAA8MIVMADO8AAowcAWyzyQlKKgCeogPzxUIAG4gY8XtX69degFCOiJcjHpR5e9H34iAbwBffiA

[eps1lon]

I cannot commit it. :D

git commit --no-verify should do.

[Lukas-Kullmann]

Thank you. I pushed the commit.

[kentcdodds]

I'm not sure of the best way to make tsc ignore the // $ExpectError comments, but we should probably figure it out before committing this otherwise anyone who changes that file will have the same problem with the commit hook.

Should we just make tsc ignore that file altogether?

[Lukas-Kullmann]

Should we just make tsc ignore that file altogether?

I think that would be the best. How would you tackle this, @kentcdodds ? Changing the kcd-scripts?
Would you say that it would be a good heuristic to say that files in a "types" folder should not be built? Maybe only if the types folder is in the project's root?

[kentcdodds]

I think we should change kcd-scripts to not typecheck check types/__tests__/**/*.* by default.

Would you be interested in doing that?

[Lukas-Kullmann]

Hey @eps1lon and @kentcdodds, I updated the version of kcd-scripts. Now the type-tests.ts file can be committed even though it has a ts-error in it. Please re-review. :)

[kentcdodds]

Thank you so much 👏

@all-contributors please add @Lukas-Kullmann for code and tests

[allcontributors]

@kentcdodds

I've put up a pull request to add @Lukas-Kullmann! 🎉

[kentcdodds]

🎉 This PR is included in version 7.5.8 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀