facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
227.61k stars 46.44k forks source link

React-test-renderer: support for portal #11565

Open alansouzati opened 6 years ago

alansouzati commented 6 years ago

Do you want to request a feature or report a bug?

Report a bug

What is the current behavior?

This test

import React from 'react';
import { createPortal } from 'react-dom';
import renderer from 'react-test-renderer';

const Drop = () => (
  createPortal(
    <div>hello</div>,
    this.dropContainer
  )
);

test('Drop renders', () => {
  const component = renderer.create(
    <div>
      <input />
      <Drop />
    </div>
  );
  const tree = component.toJSON();
  expect(tree).toMatchSnapshot();
});

fails with

Invariant Violation: Drop(...): Nothing was returned from render. This usually means a return statement is missing. Or, to render nothing, return null.

This test passes if I wrap createPortal in a container.

<div>
  {createPortal(
    <div>hello</div>,
    this.dropContainer
  )}
</div>

What is the expected behavior?

The code without the parent container works fine in the browser. So it seems that I'm adding the parent div just for the test to pass. I believe react-test-renderer should support empty returns?

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

Lastest

gaearon commented 6 years ago

Do you want to dig into why this happens?

505aaron commented 6 years ago

@gaearon I would be interested in taking a look if @alansouzati is okay with that.

alansouzati commented 6 years ago

sure. I'm ok if you want to investigate that @505aaron. Let me know if you need help.

esturcke commented 6 years ago

I've been running into problems testing portals in a CRA project with react-test-renderer as well, but with a different failure. I tried the reproduction case above, but wasn't sure how @alansouzati defined this.dropContainer. I tried:

const dropContainer = document.body.appendChild(document.createElement("div"))
const Drop = () => createPortal(<div>hello</div>, dropContainer)

The error I'm getting is

 FAIL  src/Drop.spec.js
  ● Drop renders

    TypeError: parentInstance.children.indexOf is not a function

      at appendChild (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:7183:39)
      at commitPlacement (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:4913:13)
      at commitAllHostEffects (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:5680:13)
      at HTMLUnknownElement.callCallback (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:1604:14)
      at invokeEventListeners (node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:219:27)
      at HTMLUnknownElementImpl._dispatch (node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:126:9)
      at HTMLUnknownElementImpl.dispatchEvent (node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:87:17)
      at HTMLUnknownElementImpl.dispatchEvent (node_modules/jsdom/lib/jsdom/living/nodes/HTMLElement-impl.js:36:27)
      at HTMLUnknownElement.dispatchEvent (node_modules/jsdom/lib/jsdom/living/generated/EventTarget.js:61:35)
      at Object.invokeGuardedCallbackDev (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:1643:16)
      at invokeGuardedCallback (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:1500:29)
      at commitRoot (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:5800:9)
      at performWorkOnRoot (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:6767:42)
      at performWork (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:6716:7)
      at requestWork (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:6625:7)
      at scheduleWorkImpl (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:6508:11)
      at scheduleWork (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:6470:12)
      at scheduleTopLevelUpdate (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:6941:5)
      at Object.updateContainer (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:6979:7)
      at Object.create (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:7599:18)
      at Object.<anonymous>.test (src/Drop.spec.js:8:49)
          at new Promise (<anonymous>)
      at Promise.resolve.then.el (node_modules/p-map/index.js:46:16)
          at <anonymous>
      at process._tickCallback (internal/process/next_tick.js:188:7)

  ✕ Drop renders (7ms)

Is this a separate issue or am I doing something wrong?

505aaron commented 6 years ago

Hello @esturcke, I encountered the same issue. I hit the same error as you. I also tried using a mock ref as documented https://reactjs.org/docs/test-renderer.html#ideas.

Test renderer obviously needs to remain separate from react-dom. I think mocking createPortal or extending the test renderer to make it easier to mock createPortal similar to ref mocking.

Sorry for the delay, I was on vacation.

505aaron commented 6 years ago

@esturcke @alansouzati Here is a gist that has a sample mock and snapshot test: https://gist.github.com/505aaron/d5efc2ecc622306cbcc6d3e9c1d7ee9f

@gaearon probably has a better idea/plan for supporting createPortal in react-test-renderer.

yossijacob commented 6 years ago

so @esturcke ,what did you do ?

esturcke commented 6 years ago

Hi @yossijacob. I've sort of been pushing off trying to deal with it and moved on to working on other things in the hopes that @gaearon and others might point out what I'm doing wrong or add support to react-test-renderer.

malstoun commented 6 years ago

We've encountered the same problem with parentInstance.children.indexOf is not a function error.

I dig a little and found, that parentInstance.children in case of portal is an instance of HTMLCollection, but test-renderer expects array of Instance type or TextInstance type in appendChild method.

I can work on this, but I need a little guidance @gaearon

zxydaisy commented 6 years ago

i have the same problem @gaearon

kairiruutel commented 6 years ago

I have same problem with snapshot tests for components which use ReactDOM.createPortal. I am using latest version of react-test-renderer, are there any updates related to the error (TypeError: parentInstance.children.indexOf is not a function) @gaearon ?

diasbruno commented 6 years ago

Hi, I was investigating this issue and here is what I found.

As @malstoun pointed out, the reason is an incompatibility between createPortal() and react-test-renderer (in case your problem isparentInstance.children.indexOf is not a function).

A workaround is to tricking both APIs to accept the "new container" (this is very hacky).

const ELEMENT_TYPE = 1;
const dropContainer = { children: [], nodeType: ELEMENT_TYPE };
// if necessary, you can include methods like `appendChild` to this object.

For anyone investigating this...nodeType will be checked by the isValidElement() on createPortal() and the children can be used by the react-dom-renderer internals.

12289 is a temporary fix/helper. It will export the toJSON from react-test-renderer to be used to write the test.

const modalTree = toJSON(dropContainer.children[0]);
expect(modalTree).toMatchSnapshot();

Hope this can help on further investigation of this issue.

cardamo commented 6 years ago

Speaking of workarounds. I've just added this hack to my jest snapshot tests.

ReactDOM.createPortal = node => node
reyronald commented 6 years ago

Something else that works is mocking portals in the top of the test file like this:

jest.mock('rc-util/lib/Portal')

Obviously the rc-util package is needed for this.

kebot commented 6 years ago

Here is an example of how I did, you can Mock the createPortal with your own logic.

describe('Popover', () => {
  beforeAll(() => {
    ReactDOM.createPortal = jest.fn((element, node) => {
      return element
    })
  })

  afterEach(() => {
    ReactDOM.createPortal.mockClear()
  })

  it('should render correctly with Node or Function', () => {
    const component = renderer.create(
      <Popover active trigger={<button>BUTTON</button>}>this is a Popover</Popover>
    )

    expect(component.toJSON()).toMatchSnapshot()
  })
})

Then your Snaptests will looks like this:

// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Popover should render correctly with Node or Function 1`] = `
<div
  className="popover"
>
  <button
    onClick={[Function]}
  >
    BUTTON
  </button>
  <div
    className="bottom show"
    onClick={[Function]}
    style={
      Object {
        "left": 0,
        "position": "absolute",
        "top": 0,
        "zIndex": 800,
      }
    }
  >
    this is a Popover
  </div>
</div>
`;

Any thoughts?

bvaughn commented 6 years ago

Currently, two React renderers cannot be used at the same time in the way that is being described. (A exception sort of exists for react-art and react-dom- where we intend to support cross render portals, but we have not yet built this.)

The reason this fails is mentioned above- the container types are different. react-dom expects a DOM element (e.g. HTMLDivElement) which children are appended to using container.appendChild(). react-test-renderer expects a wrapper object with a children array. It adds children by calling container.children.push().

I think this should be possible to support in a better way. See PR #12895.

gaearon commented 6 years ago

For context, latest thinking on this is: https://github.com/facebook/react/pull/12895#issuecomment-392854632

luillyfe commented 5 years ago

@kebot @cardamo ReactDOM.createPortal = node => node does not work in my case. I am getting this: "createPortal" is read-only.

I made it work,

jest.mock("react-dom", () => ({
  createPortal: node => node
}));
jest.mock("../Modal", () => () => <div id="modal" />);
ljharb commented 5 years ago

That sounds like a Flow or TypeScript error, not a javascript error.

damiangreen commented 5 years ago

trying @luillyfe's approach then gives me TypeError: ReactDOM.findDOMNode is not a function

milesj commented 4 years ago

I temporarily solved this in Rut (https://github.com/milesj/rut) by patching over the react-dom before every render, and reverting the change after the render is complete. It "works" but would be nice to support this natively.

clemente-xyz commented 4 years ago

Still having the issue. Any updates guys?

ZeroDarkThirty commented 4 years ago

Anybody getting the following error TypeError: Cannot read property 'Events' of undefined on @luillyfe's solution can try the following:

jest.mock("react-dom", () => {
    const original = jest.requireActual("react-dom");
    return {
        ...original,
        createPortal: (node: any) => node,
    };
});
stale[bot] commented 4 years ago

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

ljharb commented 4 years ago

bump

nathan5x commented 4 years ago

bump +

blissdev commented 4 years ago

Still encountering this issue.

RyanPWalker commented 4 years ago

For those of you who get the error Cannot read property 'tag' of null when mocking the createPortal, assign the original createPortal implementation to a variable, mock the implementation and run your test, then reassign it to the original implementation after the test. Only downside is you would have to do this for every test where you need it.

    it('Renders correctly', () => {
        const oldPortal = ReactDOM.createPortal;
        ReactDOM.createPortal = node => node; // for components that use Portal
        const component = renderer.create(<Portal>{children}</Portal>);
        const tree = component.toJSON();

        expect(tree).toMatchSnapshot();

        ReactDOM.createPortal = oldPortal;
    });
faccudiaz commented 4 years ago

bump

tqwewe commented 4 years ago

I'm almost there.. just receiving the following error:

TypeError: Cannot set property 'scrollTop' of null

Anyone came across this?

stale[bot] commented 3 years ago

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

ljharb commented 3 years ago

bump

KorySchneider commented 3 years ago

bump, none of the above solutions worked

gandhirahul commented 3 years ago

bump

exaucae commented 3 years ago

@kebot @cardamo, getting type error with your solutions in typescript: type ReactNode is not assignable to type ReactPortal.

cardamo commented 3 years ago

@chrys-exaucet both of them are in vanilla js, not TypeScript. Since it's a workaround anyway, just make TS to ignore these types like this:

// @ts-ignore
ReactDOM.createPortal = node => node;

or like this

ReactDOM.createPortal = node => node as ReactPortal;
cardamo commented 3 years ago

I've just tried to reproduce the issue and all seems to work fine with React 17, specifically:

    "@testing-library/jest-dom": "^5.11.9",
    "@testing-library/react": "^11.2.5",
    "react": "^17.0.1",

Was it fixed in 17 or earlier?

ahummel25 commented 3 years ago

I still have the error mentioned in the below issue. Is there a fix for this when using Material UI? Seems MUI is dependent on this.

ForwardRef(Fade)(...): Nothing was returned from render. This usually means a return statement is missing. Or, to render nothing, return null.

https://github.com/mui-org/material-ui/issues/12237

exaucae commented 3 years ago

@cardamo, I experience issues with your specified versions. Here's the related sandbox.

alpgokcek commented 3 years ago

bump

aventide commented 3 years ago

bump

exaucae commented 3 years ago

Bump!

bvaughn commented 2 years ago

Folks: Please stop bumping this issue, or we will lock it. The core team is aware of it.

I will tag it as "React Core Team" so no bot attempts to close it. In return, please trust us to prioritize it appropriately.

kadikbecerrasoliz commented 2 years ago

Here is an example of how I did, you can Mock the createPortal with your own logic.

describe('Popover', () => {
  beforeAll(() => {
    ReactDOM.createPortal = jest.fn((element, node) => {
      return element
    })
  })

  afterEach(() => {
    ReactDOM.createPortal.mockClear()
  })

  it('should render correctly with Node or Function', () => {
    const component = renderer.create(
      <Popover active trigger={<button>BUTTON</button>}>this is a Popover</Popover>
    )

    expect(component.toJSON()).toMatchSnapshot()
  })
})

Then your Snaptests will looks like this:

// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Popover should render correctly with Node or Function 1`] = `
<div
  className="popover"
>
  <button
    onClick={[Function]}
  >
    BUTTON
  </button>
  <div
    className="bottom show"
    onClick={[Function]}
    style={
      Object {
        "left": 0,
        "position": "absolute",
        "top": 0,
        "zIndex": 800,
      }
    }
  >
    this is a Popover
  </div>
</div>
`;

Any thoughts?

Thank you. I can say for certain that mocking the portal function works on a pure javascript environment.

ediazm commented 2 years ago

bump!

matthew-dean commented 2 years ago

@cardamo For your solution, or others doing this:

ReactDOM.createPortal = node => node as ReactPortal

Where are ReactDOM and ReactPortal being imported from? No one seems to say.

matthew-dean commented 2 years ago

Note: this worked based on a Stack Overflow answer:

jest.mock('react-dom', () => ({
  // @ts-ignore
  ...jest.requireActual('react-dom'),
  createPortal: node => node
}))
abner81 commented 2 years ago

Note: this worked based on a Stack Overflow answer:

jest.mock('react-dom', () => ({
  // @ts-ignore
  ...jest.requireActual('react-dom'),
  createPortal: node => node
}))

it really worked! Simple and more efficient solution. Thank you very much!

aris-b commented 1 year ago

You can also auto-mock react-dom by creating a file in ./__mocks__/react-dom.ts and adding the following:

const originalModule = jest.requireActual('react-dom');

module.exports = {
  ...originalModule,
  createPortal: node => node
};

export {};