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

[#610] waitForElementToBeRemoved: fix return type #631

Conversation

tjefferson08
Copy link
Contributor

What:
A possible fix for issue #610 -> a refinement to the return value (and TS type) for waitForElementToBeRemoved

Why:
There's a mismatch between implementation and type signature (and docs)

How:
I've updated the return type to Promise<void> (and implemented by returning undefined)

For more details, please refer to the commit message(s)!

Checklist:

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

There's a couple options here, as issue testing-library#610 enumerates nicely.

1) Resolve to the type param T

Keeping references to removed elements is a probable source of bugs and
confusing tests. A reference should be easy enough to obtain with other
test utilities; I submit that there's no added value in providing it
here.

2) Resolve to boolean (or literal `true`)

Not a bad choice by any means, but not particularly meaningful (the
fulfillment of the promise is signal enough IMO)

3) Resolve to something empty
This is what I've gone with. We are _literally_ waiting for removal,
after all :)

Fixes testing-library#610
@codesandbox-ci
Copy link

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 6f3ea3d:

Sandbox Source
nifty-pasteur-vwtsk Configuration

@codecov
Copy link

codecov bot commented Jun 13, 2020

Codecov Report

Merging #631 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #631   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           24        24           
  Lines          565       565           
  Branches       141       141           
=========================================
  Hits           565       565           
Impacted Files Coverage Δ
src/wait-for-element-to-be-removed.js 100.00% <100.00%> (ø)

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 0158f0d...6f3ea3d. Read the comment docs.

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks 👍

@kentcdodds kentcdodds merged commit 418581f into testing-library:master Jun 14, 2020
@kentcdodds
Copy link
Member

@all-contributors please add @tjefferson08 for code and tests

@allcontributors
Copy link
Contributor

@kentcdodds

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

@kentcdodds
Copy link
Member

🎉 This PR is included in version 7.14.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@tjefferson08 tjefferson08 deleted the pr/fix-waitforelementtoberemoved-return-type branch June 14, 2020 03:49
sandersn added a commit to DefinitelyTyped/DefinitelyTyped that referenced this pull request Jun 29, 2020
waitForElementToBeRemoved had the wrong return type, and it got fixed in
testing-library/dom-testing-library#631

This PR updates @testing-library/vue, which depends on
@testing-library/dom.
sandersn added a commit to DefinitelyTyped/DefinitelyTyped that referenced this pull request Jun 29, 2020
waitForElementToBeRemoved had the wrong return type, and it got fixed in
testing-library/dom-testing-library#631

This PR updates @testing-library/vue, which depends on
@testing-library/dom.
ngbrown pushed a commit to ngbrown-forks/DefinitelyTyped that referenced this pull request Jul 11, 2020
waitForElementToBeRemoved had the wrong return type, and it got fixed in
testing-library/dom-testing-library#631

This PR updates @testing-library/vue, which depends on
@testing-library/dom.
danielrearden pushed a commit to danielrearden/DefinitelyTyped that referenced this pull request Sep 22, 2020
waitForElementToBeRemoved had the wrong return type, and it got fixed in
testing-library/dom-testing-library#631

This PR updates @testing-library/vue, which depends on
@testing-library/dom.
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 this pull request may close these issues.

None yet

2 participants