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

React-test-renderer: support for portal #11565

Open
alansouzati opened this issue Nov 15, 2017 · 49 comments
Open

React-test-renderer: support for portal #11565

alansouzati opened this issue Nov 15, 2017 · 49 comments
Labels

Comments

@alansouzati
Copy link
Contributor

alansouzati commented Nov 15, 2017

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
Copy link
Collaborator

gaearon commented Nov 18, 2017

Do you want to dig into why this happens?

@505aaron
Copy link

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

@alansouzati
Copy link
Contributor Author

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

@esturcke
Copy link

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
Copy link

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
Copy link

505aaron commented Dec 1, 2017

@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
Copy link

so @esturcke ,what did you do ?

@esturcke
Copy link

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
Copy link

malstoun commented Dec 15, 2017

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
Copy link

i have the same problem @gaearon

@kairiruutel
Copy link

kairiruutel commented Jan 22, 2018

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
Copy link

diasbruno commented Feb 27, 2018

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
Copy link

cardamo commented Feb 27, 2018

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

ReactDOM.createPortal = node => node

@reyronald
Copy link

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
Copy link

kebot commented Apr 10, 2018

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 bvaughn self-assigned this May 23, 2018
@bvaughn
Copy link
Contributor

bvaughn commented May 23, 2018

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
Copy link
Collaborator

gaearon commented Aug 17, 2018

For context, latest thinking on this is: #12895 (comment)

@stale
Copy link

stale bot commented Oct 4, 2020

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!

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Oct 4, 2020
@ljharb
Copy link
Contributor

ljharb commented Oct 4, 2020

bump

@stale stale bot removed the Resolution: Stale Automatically closed due to inactivity label Oct 4, 2020
@KorySchneider
Copy link

bump, none of the above solutions worked

richardxia added a commit to ShelterTechSF/sheltertech.org that referenced this issue Nov 16, 2020
richardxia added a commit to ShelterTechSF/sheltertech.org that referenced this issue Nov 16, 2020
richardxia added a commit to ShelterTechSF/sheltertech.org that referenced this issue Nov 16, 2020
richardxia added a commit to ShelterTechSF/sheltertech.org that referenced this issue Nov 17, 2020
@gandhirahul
Copy link

bump

@exaucae
Copy link

exaucae commented Apr 24, 2021

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

@cardamo
Copy link

cardamo commented Apr 24, 2021

@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
Copy link

cardamo commented Apr 24, 2021

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
Copy link

ahummel25 commented Apr 28, 2021

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.

mui/material-ui#12237

@exaucae
Copy link

exaucae commented May 3, 2021

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

@alpgokcek
Copy link

bump

1 similar comment
@aventide
Copy link

bump

@exaucae
Copy link

exaucae commented Jul 9, 2021

Bump!

@bvaughn
Copy link
Contributor

bvaughn commented Oct 1, 2021

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.

@bvaughn bvaughn added the React Core Team Opened by a member of the React Core Team label Oct 1, 2021
@kadikbecerrasoliz
Copy link

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
Copy link

ediazm commented Feb 22, 2022

bump!

@matthew-dean
Copy link

@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
Copy link

Note: this worked based on a Stack Overflow answer:

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

@abner81
Copy link

abner81 commented May 5, 2022

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
Copy link

aris-b commented Dec 4, 2022

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 {};

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

No branches or pull requests