Skip to content

Unable to find the "window" object for the given node for input event #863

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
MattHeard opened this issue Dec 27, 2020 · 8 comments · Fixed by #1089
Closed

Unable to find the "window" object for the given node for input event #863

MattHeard opened this issue Dec 27, 2020 · 8 comments · Fixed by #1089
Labels
enhancement New feature or request

Comments

@MattHeard
Copy link

  • @testing-library/dom version: 7.28.1
  • Testing Framework and version: jest 7.0.14
  • DOM Environment: jsdom 16.4.0

Relevant code or config

import React from 'react';
import { createEvent, render } from '@testing-library/react';

describe('input', () => {
  it('creates an event', () => {
    const input: HTMLInputElement = render(<input />);
    const event: React.FormEvent<HTMLInputElement> = createEvent.change(input, {})
  });
});

What you did:

I ran the above test with npm test (which runs jest).

What happened:

Unable to find the "window" object for the given node. Please file an issue with the code that's causing you to see this error: https://github.com/testing-library/dom-testing-library/issues/new

Reproduction repository:
https://github.com/testing-library/dom-testing-library-template

Problem description:

Suggested solution:

@eps1lon
Copy link
Member

eps1lon commented Dec 27, 2020

Thanks for the report.

We should check if we're actually given an HTMLElement because that's not the case here. What you probably wanted to do is

-const input = render(<input />);
+const { getByRole } = render(<input />);
+const input = getByRole("textbox");
const event = createEvent.change(input, {});

@eps1lon eps1lon added the enhancement New feature or request label Dec 27, 2020
@MattHeard
Copy link
Author

That's right. Sorry. I'm always confusing HtmlElement with HtmlInputElement and forgetting the getByRole query.

@mathiassoeholm
Copy link

@eps1lon If I understand the code correctly, you're already checking in getWindowFromNode, whether node is a DOM node.

I think the problem here is that the error message it a bit cryptic. It doesn't hint that the node itself is invalid. Perhaps getWindowFromNode should throw an error telling that node itself is not a valid DOM Node? :-)

@eps1lon
Copy link
Member

eps1lon commented Dec 29, 2020

If I understand the code correctly, you're already checking in getWindowFromNode, whether node is a DOM node.

It doesn't seem that way.

Perhaps getWindowFromNode should throw an error telling that node itself is not a valid DOM Node? :-)

Isn't that directly contradicting the statement you made above?

The question is if we do want to extend runtime type-checking to fireEvent.change. Generally, I tend to lean towards using static type-checking instead which catches these mistakes earlier and we don't need to maintain multiple type-checkers. Right now we already have TypeScript in place.

@mathiassoeholm
Copy link

mathiassoeholm commented Dec 29, 2020

To be clear, my suggestion is simply to change the error message from

"Unable to find the "window" object for the given node ..."

to something like

`Expected node to be a DOM Node, but got ${getTypeName(node)}`

and perhaps throw TypeError instead of Error.

The reason I mention that you're already checking whether node is a DOM Node inside getWindowFromNode is because of:

} else if (node.ownerDocument && node.ownerDocument.defaultView) {
    // node is a DOM node
    return node.ownerDocument.defaultView

It literally says in the comment "node is a DOM node".

I can definitely see how adding runtime type-checking to fireEvent.change itself could result in a more helpful error, but I agree on your comment about preferring static type checking and not having to maintain both.

@Sawtaytoes
Copy link

I don't have static type checking because I'm using JavaScript. Is TypeScript a requirement for dom-testing-library or am I misunderstanding what you mean?

Also, this is a testing library. I'd argue that it's more important to have better error messages to make it easy for people understand and fix broken code or tests rather than disable the test because they're in a hurry and have absolutely not clue what's wrong.

@jh0l
Copy link

jh0l commented Aug 31, 2021

I had this error too but my issue was passing a function to fireEvent.click instead of a DOM node directly.
This error message through me off by quite a bit.

-fireEvent.click(() => screen.getByText('Preview Book'));
+fireEvent.click(screen.getByText('Preview Book'));

@MatanBobi
Copy link
Member

🎉 This fix is included in version 8.11.3 🎉

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
enhancement New feature or request
Projects
None yet
6 participants