ReactTestRenderer and ReactDOM portals #12895

bvaughn · 2018-05-23T22:43:36Z

Resolves issue #11565

Adds a warning when a React DOM portal is passed to ReactTestRenderer.

The behavior described below was reverted via 807e623 + 963ed6b + 444619b + db42945.

ReactTestRenderer should not runtime error when rendering content that includes a portal created by ReactDOM.createPortal(). This PR introduces a new feature flag- used only by the test renderer- that converts portals to fragments, enabling them to be rendered and represented as JSON by the test renderer.

Before

This test fails:

ReactTestRenderer.create(ReactDOM.createPortal('foo', container));

With a confusing error:

TypeError: parentInstance.children.indexOf is not a function

I think we should either provide a more helpful warning (see 7d57c8a) or support it enough to at least render a JSON representation of the tree (see current implementation).

After

Test

const container = document.createElement('div');
let rendered = ReactTestRenderer.create(
  <div>
    {ReactDOM.createPortal(<span>ReactDOM portal</span>, container)}
  </div>,
);
expect(rendered.toJSON()).toMatchSnapshot();

rendered.update(
  <div>
    <span>ReactTestRenderer span</span>
    {ReactDOM.createPortal(<span>ReactDOM portal</span>, container)}
  </div>,
);
expect(rendered.toJSON()).toMatchSnapshot();

Snapshot

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

exports[`ReactTestRenderer should support ReactDOM.createPortal 1`] = `
<div>
  <span>
    ReactDOM portal
  </span>
</div>
`;

exports[`ReactTestRenderer should support ReactDOM.createPortal 2`] = `
<div>
  <span>
    ReactTestRenderer span
  </span>
  <span>
    ReactDOM portal
  </span>
</div>
`;

gaearon · 2018-05-24T00:16:54Z

If you did want to test a portal though and you couldn't resort to mocks, what would the API be like?

We could teach the test renderer to map "unexpected" containers it receives to test renderer containers. Then maybe you could even assert on what's rendered in the portal.

bvaughn · 2018-05-24T01:05:17Z

Why couldn't you mock ReactDOM.createPortal (if your tests are using test renderer)? That seems reasonable to me.

We could teach the test renderer to map "unexpected" containers it receives to test renderer containers. Then maybe you could even assert on what's rendered in the portal.

Patching test renderer isn't sufficient though. This would also break DOM too.

gaearon · 2018-05-24T01:32:19Z

Why couldn't you mock ReactDOM.createPortal (if your tests are using test renderer)? That seems reasonable to me.

IMO it's nice to have first-class APIs for common tasks and not rely on hijacking exports when we can. Mocking is practical but maybe this points to a bigger issue? We also have a general cross-renderer portal problem although that's probably different.

This would also break DOM too.

Can you explain?

bvaughn · 2018-05-24T14:36:10Z

Mocking is practical but maybe this points to a bigger issue?

Well, I think the bigger issue is that it's not supported to mix renderers. You wouldn't try to mix e.g. ReactNative and ReactDOM.

Can you explain?

I believe I misread your original suggestion.

If we try to append a test renderer instance to a DOM parent, the DOM element will error because of the invalid child type, but I guess that wasn't what you were suggesting.

I'm not sure how we could do the mapping you suggest. I will give it a little thought.

bvaughn · 2018-05-24T20:24:34Z

Ooh, check out this. 😄

Test

const container = document.createElement('div');
let rendered = ReactTestRenderer.create(
  <div>
    {ReactDOM.createPortal(<span>ReactDOM portal</span>, container)}
  </div>,
);
expect(rendered.toJSON()).toMatchSnapshot();

rendered.update(
  <div>
    <span>ReactTestRenderer span</span>
    {ReactDOM.createPortal(<span>ReactDOM portal</span>, container)}
  </div>,
);
expect(rendered.toJSON()).toMatchSnapshot();

Snapshot

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

exports[`ReactTestRenderer should support ReactDOM.createPortal 1`] = `
<div>
  <span>
    ReactDOM portal
  </span>
</div>
`;

exports[`ReactTestRenderer should support ReactDOM.createPortal 2`] = `
<div>
  <span>
    ReactTestRenderer span
  </span>
  <span>
    ReactDOM portal
  </span>
</div>
`;

bvaughn · 2018-05-24T20:28:59Z

packages/react-reconciler/src/ReactFiber.js

@@ -493,7 +496,8 @@ export function createFiberFromPortal(
  expirationTime: ExpirationTime,
 ): Fiber {
  const pendingProps = portal.children !== null ? portal.children : [];
-  const fiber = createFiber(HostPortal, pendingProps, portal.key, mode);
+  const tag = convertPortalsToFragments ? Fragment : HostPortal;


This is gross 😬

sebmarkbage · 2018-05-29T16:26:27Z

We don't support DOM refs or findDOMNode which are DOM specific things in test renderer. So it's not supposed to be fully DOM compatible. Why does this need to be?

We wanted to have a way to do isomorphic portals which would be supported.

bvaughn · 2018-05-29T16:33:30Z

We don't support DOM refs or findDOMNode which are DOM specific things in test renderer. So it's not supposed to be fully DOM compatible. Why does this need to be?

Refs in general aren't supported by the test renderer, nor are the did-mount/did-update lifecycles- so I feel like those two don't really confuse or surprise people. We just side step them entirely.

Portals seem more useful/helpful to support, because you might be testing a component- that has a dependent that uses ReactDom.createPortal- without even realizing it. (I encountered this within Facebook a few days ago. Plus there are external users on #11565 making a case for this.)

We could just warn about this for now, as my original commit did.

We wanted to have a way to do isomorphic portals which would be supported.

Seems much larger scope than this. Even if we did this, we could always replace it with that in the future?

sebmarkbage · 2018-05-29T17:00:35Z

I feel like when we support something, we commit to doing it properly. Even if it just looks like we are. (Like gDSFP)

So I kind of prefer the warning solution until we have a fleshed out strategy which may or may not depend on isomorphic portals.

The test renderer was kind of intended to not be used with deep dependents. Originally we only supported the shallow renderer and then the test renderer got supported in the Enzyme world with the intention that complex components would be mocked. Otherwise they don't work for updates anyway since updates can rely on setState in CDM and refs.

pull-bot · 2018-05-29T17:37:08Z

Details of bundled changes.

Comparing: bf1abf4...8b865c3

react-test-renderer

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-test-renderer.development.js	+0.1%	-0.1%	370.77 KB	371.09 KB	81.3 KB	81.2 KB	UMD_DEV
react-test-renderer.development.js	+0.1%	-0.1%	366.9 KB	367.22 KB	80.34 KB	80.24 KB	NODE_DEV
ReactTestRenderer-dev.js	+0.1%	+0.2%	371.96 KB	372.35 KB	79.18 KB	79.3 KB	FB_WWW_DEV

Generated by 🚫 dangerJS

bvaughn · 2018-05-29T18:05:46Z

Okay @sebmarkbage 👍

I have reverted my changes to "support" it. We're back to the added warning only.

bvaughn · 2018-06-13T14:14:23Z

Bump. Do we want to add a warning for this? 😄

brokentone · 2018-07-19T22:29:08Z

Any update on this PR? I think it would make at least complex error situations I've hit with Enzyme more clear while we wait for a more robust reconciliation

facebook-github-bot · 2018-07-25T21:34:40Z

Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired.

Before we can review or merge your code, we need you to email cla@fb.com with your details so we can update your status.

bvaughn · 2018-07-26T01:07:55Z

Go home, @facebook-circleci-bot

facebook-github-bot · 2018-07-26T02:42:43Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

sebmarkbage · 2018-08-15T23:58:13Z

packages/react-test-renderer/src/ReactTestHostConfig.js

@@ -57,6 +58,15 @@ export function appendChild(
  parentInstance: Instance | Container,
  child: Instance | TextInstance,
 ): void {
+  if (__DEV__) {
+    warning(
+      typeof parentInstance.children.indexOf === 'function',


Array.isArray(...) might be better. More specific.

bvaughn requested review from gaearon and acdlite May 23, 2018 22:43

facebook-github-bot added the CLA Signed label May 23, 2018

bvaughn force-pushed the test-renderer-portal-warning branch from 13c6ac3 to 7d57c8a Compare May 23, 2018 22:55

bvaughn changed the title ~~Warn about ReactDOM.createPortal usage within ReactTestRenderer~~ ReactTestRenderer and ReactDOM.createPortal May 24, 2018

bvaughn force-pushed the test-renderer-portal-warning branch from 660d69f to b8a2892 Compare May 24, 2018 20:26

bvaughn mentioned this pull request May 24, 2018

React-test-renderer: support for portal #11565

Open

bvaughn commented May 24, 2018

View reviewed changes

bvaughn changed the title ~~ReactTestRenderer and ReactDOM.createPortal~~ ReactTestRenderer renders ReactDOM portals May 24, 2018

bvaughn requested review from sophiebits and flarnie May 24, 2018 21:33

bvaughn force-pushed the test-renderer-portal-warning branch from b8a2892 to 93eeb04 Compare May 25, 2018 21:54

bvaughn changed the title ~~ReactTestRenderer renders ReactDOM portals~~ ReactTestRenderer and ReactDOM portals May 29, 2018

sebmarkbage approved these changes Aug 15, 2018

View reviewed changes

Warn about ReactDOM.createPortal usage within ReactTestRenderer

8b865c3

bvaughn force-pushed the test-renderer-portal-warning branch from e1f87ed to 8b865c3 Compare August 20, 2018 16:45

bvaughn merged commit d670bdc into facebook:master Aug 20, 2018

bvaughn deleted the test-renderer-portal-warning branch August 20, 2018 17:03

layershifter mentioned this pull request Aug 22, 2018

Test with jest Semantic-Org/Semantic-UI-React#3100

Closed

gaearon mentioned this pull request Sep 5, 2018

Changelog for 16.5.0 #13571

Merged

dependencies bot mentioned this pull request Oct 1, 2018

Update react-dom in /modules/frontend from 16.3.0 to 16.5.2 project-accelerate/accelerate#277

Open

This was referenced Nov 1, 2018

Update react-dom in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#253

Closed

Update react in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#254

Closed

This was referenced Dec 1, 2018

Update react-test-renderer in /modules/frontend from 16.3.0 to 16.6.3 project-accelerate/accelerate#312

Open

Update react in /modules/frontend from 16.3.0 to 16.6.3 project-accelerate/accelerate#313

Open

NMinhNguyen referenced this pull request in enzymejs/react-shallow-renderer Jan 29, 2020

Warn about ReactDOM.createPortal usage within ReactTestRenderer (#12895)

257e3cb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ReactTestRenderer and ReactDOM portals #12895

ReactTestRenderer and ReactDOM portals #12895

bvaughn commented May 23, 2018 •

edited

gaearon commented May 24, 2018 •

edited

bvaughn commented May 24, 2018

gaearon commented May 24, 2018

bvaughn commented May 24, 2018

bvaughn commented May 24, 2018 •

edited

bvaughn May 24, 2018

sebmarkbage commented May 29, 2018

bvaughn commented May 29, 2018 •

edited

sebmarkbage commented May 29, 2018

pull-bot commented May 29, 2018 •

edited

react-test-renderer

bvaughn commented May 29, 2018

bvaughn commented Jun 13, 2018

brokentone commented Jul 19, 2018

facebook-github-bot commented Jul 25, 2018

bvaughn commented Jul 26, 2018

facebook-github-bot commented Jul 26, 2018

sebmarkbage Aug 15, 2018

ReactTestRenderer and ReactDOM portals #12895

ReactTestRenderer and ReactDOM portals #12895

Conversation

bvaughn commented May 23, 2018 • edited

Before

After

Test

Snapshot

gaearon commented May 24, 2018 • edited

bvaughn commented May 24, 2018

gaearon commented May 24, 2018

bvaughn commented May 24, 2018

bvaughn commented May 24, 2018 • edited

Test

Snapshot

bvaughn May 24, 2018

Choose a reason for hiding this comment

sebmarkbage commented May 29, 2018

bvaughn commented May 29, 2018 • edited

sebmarkbage commented May 29, 2018

pull-bot commented May 29, 2018 • edited

react-test-renderer

bvaughn commented May 29, 2018

bvaughn commented Jun 13, 2018

brokentone commented Jul 19, 2018

facebook-github-bot commented Jul 25, 2018

bvaughn commented Jul 26, 2018

facebook-github-bot commented Jul 26, 2018

sebmarkbage Aug 15, 2018

Choose a reason for hiding this comment

bvaughn commented May 23, 2018 •

edited

gaearon commented May 24, 2018 •

edited

bvaughn commented May 24, 2018 •

edited

bvaughn commented May 29, 2018 •

edited

pull-bot commented May 29, 2018 •

edited