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(render): Default to HTMLElement in returned container #868

Merged

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Jan 26, 2021

What:

Fixes #834 (comment)

Why:

By default we create a div but declare that the type is Element. This is not specific enough for most use cases since e.g. offsetHeight isn't implemented in Element

How:

Adding a type parameter as anticipated in #833 (comment)

Checklist:

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

@eps1lon eps1lon added bug Something isn't working TypeScript Related to TypeScript. Note: only certain maintainers handle TypeScript labeled issues. labels Jan 26, 2021
@@ -35,13 +36,18 @@ export async function testPureRender() {

// helpers
const {container, rerender, debug} = page
expectType<HTMLElement, typeof container>(container)
Copy link
Member Author

Choose a reason for hiding this comment

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

This was Element before this change which caused #834 (comment)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 26, 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 a61a0d5:

Sandbox Source
React Configuration
react-testing-library-examples Configuration

@codecov
Copy link

codecov bot commented Jan 26, 2021

Codecov Report

Merging #868 (1a5ec78) into master (2a3be2c) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #868   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines          140       140           
  Branches        26        26           
=========================================
  Hits           140       140           

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 2a3be2c...a61a0d5. Read the comment docs.

@eps1lon eps1lon marked this pull request as ready for review January 26, 2021 21:24
@nickserv
Copy link
Member

nickserv commented Jan 27, 2021

Out of curiosity, why not continue to default the return type to Element so that SVGs are implicitly supported (even if TypeScript can't infer it properly) and let the user cast it to HTMLElement when needed? I can help with PRs to make the types more compatible with functions that take elements.

Alternatively, could we release this as a temporary fix and then go back after a major release? I think it's valuable to have a more generic return type for accuracy with React (as it can render both HTML and SVG elements).

types/test.tsx Outdated
Comment on lines 44 to 50
render(<div />, options)
const {container: returnedContainer} = render(<div />, options)
// Unclear why TypeScript infers `HTMLElement` here when the the input `container` is `HTMLDivElement`
// Hopefully this breaks someday and we can switch to
// expectType<HTMLDivElement, typeof returnedContainer>(returnedContainer)
expectType<HTMLElement, typeof returnedContainer>(returnedContainer)
Copy link
Member

Choose a reason for hiding this comment

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

This is a limitation of TypeScript's JSX syntax support. JSX elements unfortunately can't be inferred to a specific element type. You'd have to use the React.createElement syntax, though it's not recommended in most cases.

So for now, this test is accurate, though you could avoid JSX if you wanted to still test the regression.

Copy link
Member Author

Choose a reason for hiding this comment

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

The <div /> is irrelevant here as far as I know though definitely confusing. The type should be inferred from options.

The confusing part is why it works for SVGSVGElement but not HTMLDivElement.

Let me replace <div /> with <button /> to check if your assumption is true.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, I missed that, I'm curious what happens

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed. Is it now clearer that the container is not inferred from the rendered UI but the options?

@eps1lon
Copy link
Member Author

eps1lon commented Jan 28, 2021

Out of curiosity, why not continue to default the return type to Element so that SVGs are implicitly supported (even if TypeScript can't infer it properly)

I expanded the test for svg containers to confirm this. They now return SVGSVGElement instead of Element. So they're still assignable to Element.

Alternatively, could we release this as a temporary fix and then go back after a major release?

Why would we go back?

types/test.tsx Outdated
return {container, rerender, debug}
}

export function testRenderOptions() {
const container = document.createElement('div')
const options = {container}
render(<div />, options)
const {container: returnedContainer} = render(<button />, options)
// Unclear why TypeScript infers `HTMLElement` here when the the input `container` is `HTMLDivElement`.
Copy link
Member

@timdeschryver timdeschryver Jan 29, 2021

Choose a reason for hiding this comment

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

If we'd swap the render methods in the declaration file, this would infer correctly (it uses the first one).
I'm unfamiliar with the types for RTL, so I'm not sure if this would break something, or if we could maybe entirely remove the other render type.

Suggested change
// Unclear why TypeScript infers `HTMLElement` here when the the input `container` is `HTMLDivElement`.
// Unclear why TypeScript infers `HTMLElement` here when the input `container` is `HTMLDivElement`.

Copy link
Member Author

Choose a reason for hiding this comment

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

@timdeschryver Awesome, thanks! Fixed in a61a0d5 (#868)

Copy link
Member

Choose a reason for hiding this comment

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

I'm glad I could help 🙂

@eps1lon
Copy link
Member Author

eps1lon commented Feb 2, 2021

Merging despite pending codecov/project status. The codecov comment is up-to-date and reports the desired 100%.

@eps1lon eps1lon merged commit 81c2de9 into testing-library:master Feb 2, 2021
@eps1lon eps1lon deleted the feat/types-default-container branch February 2, 2021 09:30
@github-actions
Copy link

github-actions bot commented Feb 2, 2021

🎉 This PR is included in version 11.2.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

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 Related to TypeScript. Note: only certain maintainers handle TypeScript labeled issues.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants