Skip to content

getByPlaceholderText Should Also Return Type HTMLInputElement? #65

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

Closed
duncanleung opened this issue Jul 8, 2018 · 10 comments
Closed

getByPlaceholderText Should Also Return Type HTMLInputElement? #65

duncanleung opened this issue Jul 8, 2018 · 10 comments

Comments

@duncanleung
Copy link

  • dom-testing-library version: 2.7.0
  • react version: 16.4.1
  • redux-form version: 7.4.2
  • node version: 8.11.3
  • npm (or yarn) version: 6.1.0

Relevant code or config:

import React from "react";
import { renderWithRedux, generate } from "test-utils";
import { cleanup, fireEvent } from "react-testing-library";

import LoginFormContainer from "../LoginFormContainer";

afterEach(cleanup);

describe("LoginFormContainer", () => {
  test("Submits Login with email and password", () => {
    //Arrange--------------
    const fakeUser = generate.loginForm();

    let submitLogin = jest.fn();
    const props = { submitLogin };

    const { getByText, getByPlaceholderText } = renderWithRedux(
      <LoginFormContainer {...props} />
    );

    const emailNode = getByPlaceholderText("E-mail address");
    const passwordNode = getByPlaceholderText("Password");

    //Act--------------
    emailNode.value = fakeUser.email;
              ^^^^^ [ts] Property 'value' does not exist on type 'HTMLElement'.
    passwordNode.value = fakeUser.password;
                 ^^^^^ [ts] Property 'value' does not exist on type 'HTMLElement'.

    fireEvent.change(emailNode);
    fireEvent.change(passwordNode);

    fireEvent.click(getByText("Login"));

    //Assert--------------
    expect(submitLogin).toHaveBeenCalledTimes(1);
    expect(submitLogin).toHaveBeenCalledWith(fakeUser);
  });
});

renderWithRedux is the same as the examples from react-testing-library

import React from "react";
import { createStore } from "redux";
import { Provider } from "react-redux";
import { render } from "react-testing-library";

import reducer from "../services/reducer";

export default function renderWithRedux(
  ui: React.ReactNode,
  { initialState, store = createStore(reducer, initialState) } = {}
) {
  return {
    ...render(<Provider store={store}>{ui}</Provider>),
    // adding `store` to the returned utilities to allow us
    // to reference it in our tests (just try to avoid using
    // this to test implementation details).
    store
  };
}

What you did:

I am using redux-form for my forms.

I want to:

  • modify the input values (email / password)
  • click "Login"
  • verify that submitLogin is fired

react-testing-library: fireEvent If you want to trigger the onChange handler of a controlled component with a different event.target.value, sending value through eventProperties won't work like it does with Simulate.
You need to change the element's value property, then use fireEvent to fire a change DOM event.

What happened:

I get TypeScript errors from the returned Nodes from getByPlaceholderText, because it returns a type HTMLElement.

    const emailNode = getByPlaceholderText("E-mail address");
    const passwordNode = getByPlaceholderText("Password");

    emailNode.value = fakeUser.email;
              ^^^^^ [ts] Property 'value' does not exist on type 'HTMLElement'.
    passwordNode.value = fakeUser.password;
                 ^^^^^ [ts] Property 'value' does not exist on type 'HTMLElement'.

Reproduction:

Problem description:

Suggested solution:

I'm not very well versed with TypeScript, but I'm wondering if updating the type definition for queries.d.ts, so that type GetByAttribute returns an intersection of HTMLElement & HTMLInputElement makes sense?

// queries.d.ts

export type GetByAttribute = (
  container: HTMLElement,
  id: Matcher,
  options?: MatcherOptions,
) => HTMLElement & HTMLInputElement
                   ^^^^^^^^^^^^^^^^^ Add intersection type?

export const getByPlaceholderText: GetByAttribute

In the meantime, I'm just typecasting:

    const emailNode = getByPlaceholderText("E-mail address") as HTMLInputElement;
    const passwordNode = getByPlaceholderText("Password") as HTMLInputElement;
@kentcdodds
Copy link
Member

Would that support a textarea as well? I'm not sure what other elements support a placeholder, but any elements that support a placeholder should be what this query should be able to return.

@kentcdodds
Copy link
Member

Also, thank you for the detailed issue! Hopefully one of our TS maintainers can jump in and help with this one :)

@duncanleung
Copy link
Author

I’ll test it out on textarea and other elements that have placeholder.

If it also works, I’ll work on a PR!

Sent with GitHawk

@jpavon
Copy link

jpavon commented Jul 8, 2018

This is a problem with all the get and queries where the returned element is unknown, this could be a solution:

// add generics in all the get and queries 
export type GetByAttribute = <T extends HTMLElement>(
  container: HTMLElement,
  id: Matcher,
  options?: MatcherOptions,
) => T

// in your code
getByPlaceholderText<HTMLInputElement>("Password")

@kentcdodds
Copy link
Member

That sounds fine with me. I don't maintain the typescript typings so anyone who wants to help maintain them is welcome to do so.

@sompylasar
Copy link
Collaborator

This is a problem with all the get and queries where the returned element is unknown

That's true.

But I think the existing typings are right, type definition should not be changed.

The element type is actually unknown. The return value is generic, HTMLElement. The callers have to narrow down the types themselves after checking which type of element they actually got from the query. It's an assumption that they got HTMLInputElement which has to be verified in runtime, they could've gotten any other HTMLElement (like a textarea), and there's no way to define the return type statically depending on the queried DOM structure that is dynamic.

@duncanleung
Copy link
Author

I see.

I don't have any counter arguments to @sompylasar

But I think the existing typings are right, type definition should not be changed.

@sompylasar, could you provide some feedback regarding my method of typecasting to satisfy TypeScript?

Is that they way you would go about it?

    const emailNode = getByPlaceholderText("E-mail address") as HTMLInputElement;
    const passwordNode = getByPlaceholderText("Password") as HTMLInputElement;

@ckhicks
Copy link

ckhicks commented Jul 25, 2018

I just had to implement that same fix - my compiler refused to pass the input element as having a value property unless I cast it as HTMLInputElement.

@sompylasar
Copy link
Collaborator

@duncanleung @ckhicks

You don't need to cast to HTMLInputElement, you need to check that whatever object you got from getByPlaceholderText is in fact an HTMLInputElement and not something else. That the element will always be an HTMLInputElement is an assumption that may be broken, typecasting just hides that and begs for runtime errors that may have been prevented.

const emailNode = getByPlaceholderText("E-mail address");
const passwordNode = getByPlaceholderText("Password");

if (emailNode instanceof HTMLInputElement && passwordNode instanceof HTMLInputElement) {
  // Do something with emailNode, passwordNode; TypeScript should narrow their types to be HTMLInputElement.
} else {
  assert(false, 'expected emailNode and passwordNode to be HTMLInputElement');
}

@duncanleung
Copy link
Author

Thanks for your time and the advice!

I'll use that approach in my test.

alexkrolick pushed a commit to alexkrolick/dom-testing-library that referenced this issue Sep 13, 2018
…ary#70)

* feat(rerender): implement rerender (testing-library#65)

* update readme
* update examples
* tests
* typescript typings

* Update index.js

* Update rerender.js

* Update rerender.js

* Update README.md

Closes testing-library#65
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants