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

fix(within): Add extra type paramater to allow reassigning in TypeScript #1077

Merged
merged 4 commits into from Jan 15, 2022

Conversation

Dennis273
Copy link
Contributor

@Dennis273 Dennis273 commented Dec 12, 2021

What:

Closes testing-library/react-testing-library#979

TS error occurs when reassigning the return value of within

import { screen, within } from "@testing-library/dom";
import "@types/jest";
import "@testing-library/jest-dom";

const navigation = screen.getByRole("navigation");
let breadcrumbs = within(navigation);
expect(breadcrumbs.getByRole("link", { name: /home/i })).toBeInTheDocument();
// ts error occurs
breadcrumbs = within(navigation);

https://www.typescriptlang.org/play?#code/JYWwDg9gTgLgBAbzgZwMZQKYYHYBo4DuwMAFsNnAL5wBmUEIcARAAIwbIzkDmAtADbAARlACGUAJ4B6ACYMmAbgBQoSLGZsJYDlIBWHGIpXho8Vu048BwsZL0HeckEaWoI2TnGyiAbsG6iXO5wALwo6FjYAHTcGDAAQhIAShD8GAAUTN5+AUHYTACUymnwIhiiMugAriBCyKGExGTY6dn+gcDuRUoYAB7aqDDpZRXVtcgxcYkpaZmC2ADWTPhI3iAYAFxwUiQMGFLAVAUFUTAQ8RgAktgAKiQYACIQqDU4Q90jlVA1dQ1EpORWr52nkikA

Why:

To avoid TS error or type-casting.

How:

The type error happens because TS tries to infer T from the variable breadcrumbs(BoundFunctions<T>) in the demo above. This is fixed by adding another generic variable K.

Checklist:

  • [N/A ] Documentation added to the
    docs site
  • Tests
  • TypeScript definitions updated
  • Ready to be merged

@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 12, 2021

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

Sandbox Source
react-testing-library-examples Configuration

@codecov
Copy link

codecov bot commented Dec 13, 2021

Codecov Report

Merging #1077 (6e6515f) into main (b4ebc80) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##              main     #1077   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        25           
  Lines          954       954           
  Branches       314       314           
=========================================
  Hits           954       954           
Flag Coverage Δ
node-12 100.00% <ø> (ø)
node-14 100.00% <ø> (ø)
node-16.9.1 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


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 b4ebc80...6e6515f. Read the comment docs.

timdeschryver
timdeschryver previously approved these changes Dec 13, 2021
Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

LGTM

I don't understand why this works so let's make sure this isn't easily
reverted before checking git-blame
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

No idea why this works. Added some more tests/comments to make sure this isn't reverted before checking git-blame.

@eps1lon eps1lon changed the title fix(within): fix ts error when reassigning fix(within): Add extra type paramater to allow reassigning in TypeScript Jan 15, 2022
@eps1lon eps1lon added bug Something isn't working TypeScript labels Jan 15, 2022
@eps1lon
Copy link
Member

eps1lon commented Jan 15, 2022

@all-contributors add @robcaldecott for bug
@all-contributors add @Dennis273 for code

@allcontributors
Copy link
Contributor

@eps1lon

I could not determine your intention.

Basic usage: @all-contributors please add @Someone for code, doc and infra

For other usages see the documentation

@eps1lon
Copy link
Member

eps1lon commented Jan 15, 2022

@all-contributors add @robcaldecott for bug

@eps1lon
Copy link
Member

eps1lon commented Jan 15, 2022

@all-contributors add @Dennis273 for code

@allcontributors
Copy link
Contributor

@eps1lon

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

@allcontributors
Copy link
Contributor

@eps1lon

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

@eps1lon eps1lon merged commit 1f143e5 into testing-library:main Jan 15, 2022
@eps1lon
Copy link
Member

eps1lon commented Jan 15, 2022

@all-contributors add @Dennis273 for code

@allcontributors
Copy link
Contributor

@eps1lon

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

@github-actions
Copy link

🎉 This PR is included in version 8.11.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Dennis273 Dennis273 deleted the pr/fix-within-typing branch January 16, 2022 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released TypeScript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeScript error reassigning a variable using within
3 participants