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

getDocumentElement null during testing #1908

Closed
raza-jamil-reckon opened this issue Oct 5, 2022 · 19 comments
Closed

getDocumentElement null during testing #1908

raza-jamil-reckon opened this issue Oct 5, 2022 · 19 comments

Comments

@raza-jamil-reckon
Copy link

I'm using https://www.radix-ui.com/docs/primitives/components/dropdown-menu in our design system which ends up using floating-ui under the hood. Everything is working fine except in jest when I click on the trigger and the popover is supposed to appear I get the following error from floating-ui/dom:

/Users/raza.jamil/dev/reckon-frontend/node_modules/.pnpm/@floating-ui/dom@0.5.4/node_modules/@floating-ui/dom/dist/floating-ui.dom.umd.js:133
    return ((isNode(node) ? node.ownerDocument : node.document) || window.document).documentElement;
                                                                                    ^

TypeError: Cannot read properties of null (reading 'documentElement')
    at getDocumentElement (/Users/raza.jamil/dev/reckon-frontend/node_modules/.pnpm/@floating-ui/dom@0.5.4/node_modules/@floating-ui/dom/dist/floating-ui.dom.umd.js:133:85)
    at Object.convertOffsetParentRelativeRectToViewportRelativeRect (/Users/raza.jamil/dev/reckon-frontend/node_modules/.pnpm/@floating-ui/dom@0.5.4/node_modules/@floating-ui/dom/dist/floating-ui.dom.umd.js:277:29)
    at detectOverflow (/Users/raza.jamil/dev/reckon-frontend/node_modules/.pnpm/@floating-ui/core@0.7.3/node_modules/@floating-ui/core/dist/floating-ui.core.umd.js:272:128)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at fn (/Users/raza.jamil/dev/reckon-frontend/node_modules/.pnpm/@floating-ui/core@0.7.3/node_modules/@floating-ui/core/dist/floating-ui.core.umd.js:825:26)
    at Object.computePosition (/Users/raza.jamil/dev/reckon-frontend/node_modules/.pnpm/@floating-ui/core@0.7.3/node_modules/@floating-ui/core/dist/floating-ui.core.umd.js:141:11)

I've narrowed down the issue to this function https://github.com/floating-ui/floating-ui/blob/master/packages/dom/src/utils/getDocumentElement.ts.

It seems that (isNode(node) ? node.ownerDocument : node.document) || window.document) can be null causing the error. If I just use optional chaining and change it to (isNode(node) ? node.ownerDocument : node.document) || window.document)?. documentElement , the test starts working.

There's some issue with the Codesandbox template provided, I can't seem to run tests in it. I'll work on it but in the meantime does this issue and the fix make sense or am I missing something?

@atomiks
Copy link
Collaborator

atomiks commented Oct 5, 2022

Strange that it would be null, this issue has not been filed before and it's used in thousands of testing setups, so I don't think this occurs by default with jest/jsdom. I would imagine there is something in your testing setup causing this?

@popperjs/core uses almost the same check and it's even more widely used, with no issues about this:

https://github.com/floating-ui/floating-ui/blob/e764f2cc893e5a9cc466af888f7eb7efa3b197f6/src/dom-utils/getDocumentElement.js

@raza-jamil-reckon
Copy link
Author

Hmm thanks, I'm having a deeper look. It's pretty strange since there are other things in the tests that use document and they seem to be working fine.

@huynhthaianhhd
Copy link

I also have the same problem when using radix-ui.

@atomiks
Copy link
Collaborator

atomiks commented Oct 12, 2022

When you log out node, or any of the checks in the function what does it say in the console when running tests?

@huynhthaianhhd
Copy link

huynhthaianhhd commented Oct 13, 2022

@raza-jamil-reckon @atomiks i solved it.

i only just wrap Popover/Dropdown by tag div and it worked.

it('render popover', () => {
    const { container } = render(
      <div>
        <Popover open={true} onOpenChange={() => {}} target={<div>Bottom popover</div>}>
          <div>Popover</div>
        </Popover>
      </div>
    )

    expect(container).toMatchSnapshot()
  })

@atomiks
Copy link
Collaborator

atomiks commented Oct 19, 2022

@raza-jamil-reckon @huynhthaianhhd if you can make a repro for me to investigate that would help and I can re-open if valid.

As it stands, it's not actionable and I am assuming it's a testing environment or Radix UI issue, as it doesn't seem to have been reported in other component libs.

Optional chaining also doesn't make sense and I'm not sure how it works because the types break with a falsy value and things expect a valid element, not null.

@atomiks atomiks closed this as completed Oct 19, 2022
@jvidalv
Copy link

jvidalv commented Oct 27, 2022

Happening to me too, and I'm also using radix-ui, I guess their abstraction (radix-popover) is doing some magic.

"jest": "29.2.1",
"jest-environment-jsdom": "29.2.1",
"next": "12.3.1",
"@radix-ui/react-popover": "1.0.2",

The first error was ResizeObserver not being defined, which makes sense, but once this is fixed the next error I get after the tests pass is this one:

TypeError: Cannot read properties of null (reading 'documentElement')
    at getDocumentElement (/Users/jvidalv/Projects/website/node_modules/@floating-ui/dom/dist/floating-ui.dom.umd.js:133:85)

@jesus-pacheco
Copy link

jesus-pacheco commented Oct 28, 2022

Hi, it seems like this error is happening when we face an unhandled error in the Popover, for me was an "invalid" selector that is actually valid but invalid for my Jest version, so I had to wrap that logic in a Try/catch to actually see the right error, so defensive code in floating UI would be appreciated!

@atomiks
Copy link
Collaborator

atomiks commented Oct 28, 2022

I don't understand how it occurs though, the types are valid and can't be made defensive

I need a small repo to download and investigate why or how it happens, and why it only happens to Radix UI users

@mkrds
Copy link

mkrds commented Nov 2, 2022

@atomiks

Hey, unfortunately I'm having the same issue which I've encountered during a migration from popper to floating-ui, I'm not using radix btw. I've managed to reproduce this issue in stackblitz - it's a copy of the Tooltip example from docs using React@17 and Jest@26 https://stackblitz.com/edit/node-qckpvp?file=index.test.jsx

@atomiks
Copy link
Collaborator

atomiks commented Nov 3, 2022

@mkrds thanks! It seems like jest@27 onwards has a different message in the StackBlitz:

Message:
Cannot read properties of undefined (reading 'body')

Does upgrading to Jest 27-29 fix the issue locally?

@atomiks
Copy link
Collaborator

atomiks commented Nov 3, 2022

OK so running some tests locally, it seems like Jest 27-29 doesn't have the issue (with jest-environment-jsdom@27-29)

For Jest 26 it looks like it's this issue: jestjs/jest#8197 (comment)

To fix it make sure you flush the microtask queue before the test finishes, this fixed it for me locally:

https://floating-ui.com/docs/react-dom#testing

Screenshot 2022-11-03 at 12 41 58 pm

@mkrds
Copy link

mkrds commented Nov 3, 2022

Hey @atomiks thanks for the reply. Yes updating Jest to 27^ fixes this issue. I'm a little bit hesitant about that as I'm one of the maintainers of a component library used by multiple projects in Sage organization and I'm afraid we might get swamped with GH issues from angry people confused why their test suites are failing with a slightly confusing error message. On the other hand Jest@26 is already 2 years so yeah, maybe it's finally time for an upgrade

@Geloosa
Copy link

Geloosa commented Feb 1, 2023

I've started to get this one with jest and jest-environment-jsdom v29.4.1 after updating @floating-ui/core and @floating-ui/dom from v1.1.0 to v1.1.1.

@sagar-ganatra
Copy link

I'm able to reproduce this error and it's due to await fn not wrapped in a try-catch block, see

This causes Node 18 to throw an error

@esimons
Copy link

esimons commented Apr 24, 2023

I'm able to reproduce this error and it's due to await fn not wrapped in a try-catch block, see

This causes Node 18 to throw an error

To expand on this, in our case it appears that Jest was tearing down the JSDOM environment while floating-ui was trying to access window.document.documentElement and that, at that point, window.document was null and triggered this unhandled promise rejection in floating-ui.

This becomes a problem when combined with the new(ish) Node behavior of process exit upon unhandled promise rejection; our tests were passing successfully, but the Jest worker process was crashing due to these circumstances during its teardown process. Very difficult to tell that it was happening after test suites had effectively completed.

A (hacky) workaround is to add an afterAll block that just delays Jest's teardown a tick until anything floating-ui related has been completely deregistered from the dom:

  afterAll(() => 
    new Promise(resolve => setTimeout(resolve, 0))
    // this also works:
    // new Promise(setImmediate)
  );

@atomiks
Copy link
Collaborator

atomiks commented Apr 24, 2023

The error is likely occurring because you aren't waiting for the microtask queue to be flushed (and thus positioning to have occurred) in your tests, as mentioned above. This means Floating UI's positioning executes in a microtask after Jest has torn down the DOM, causing the error.

In React you put await act(async () => {}) after rendering the floating element - and for general testing, I think await Promise.resolve() can achieve the same thing. So you can remove the 100ms delay (0 will do the same thing), or switch to awaiting the microtask in each test

@esimons
Copy link

esimons commented Apr 25, 2023

The error is likely occurring because you aren't waiting for the microtask queue to be flushed (and thus positioning to have occurred) in your tests, as mentioned above. This means Floating UI's positioning executes in a microtask after Jest has torn down the DOM, causing the error.

In React you put await act(async () => {}) after rendering the floating element - and for general testing, I think await Promise.resolve() can achieve the same thing. So you can remove the 100ms delay (0 will do the same thing), or switch to awaiting the microtask in each test

I don't think that's quite it, at least in our case. Out of curiosity I've tried both await Promise.resolve() and await act(async () => {}) as suggested, and neither has any impact for our particular scenario (we're using react-testing-library, which also purportedly auto-wraps its methods in act so I'd have expected that to cover us); the only thing that seems to address the issue is a setTimeout (which can be set to a 0ms delay, you're correct, and a setImmediate callback also works here -- I'll update my comment above to reflect that).

In any event, it does seem like handling this potential promise rejection would be a good thing overall for usability within the unit-testing context, where this edge case is possible.

@krutoo
Copy link

krutoo commented Jul 4, 2023

Have same issue with

{
    "jest": "^29.5.0",
    "jest-environment-jsdom": "^29.5.0",
}

and:

@floating-ui/react@0.17.0

and:

node -v
v16.15.1

act(async () => {}) after render does not help

Only workaround from message of @esimons is working

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants