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

ReactTestRenderer and ReactDOM portals #12895

Merged
merged 1 commit into from Aug 20, 2018

Conversation

@bvaughn
Copy link
Contributor

@bvaughn bvaughn commented May 23, 2018

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>
`;
@bvaughn bvaughn requested review from gaearon and acdlite May 23, 2018
@bvaughn bvaughn force-pushed the test-renderer-portal-warning branch from 13c6ac3 to 7d57c8a May 23, 2018
@gaearon
Copy link
Member

@gaearon gaearon commented May 24, 2018

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.

Loading

@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented May 24, 2018

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.

Loading

@gaearon
Copy link
Member

@gaearon gaearon commented May 24, 2018

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?

Loading

@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented May 24, 2018

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.

Loading

@bvaughn bvaughn changed the title Warn about ReactDOM.createPortal usage within ReactTestRenderer ReactTestRenderer and ReactDOM.createPortal May 24, 2018
@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented May 24, 2018

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>
`;

Loading

@bvaughn bvaughn force-pushed the test-renderer-portal-warning branch from 660d69f to b8a2892 May 24, 2018
@@ -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;
Copy link
Contributor Author

@bvaughn bvaughn May 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is gross 😬

Loading

@bvaughn bvaughn changed the title ReactTestRenderer and ReactDOM.createPortal ReactTestRenderer renders ReactDOM portals May 24, 2018
@bvaughn bvaughn requested review from sophiebits and flarnie May 24, 2018
@bvaughn bvaughn force-pushed the test-renderer-portal-warning branch from b8a2892 to 93eeb04 May 25, 2018
@sebmarkbage
Copy link
Member

@sebmarkbage sebmarkbage commented May 29, 2018

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.

Loading

@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented May 29, 2018

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?

Loading

@sebmarkbage
Copy link
Member

@sebmarkbage sebmarkbage commented May 29, 2018

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.

Loading

@pull-bot
Copy link

@pull-bot pull-bot commented May 29, 2018

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

Loading

@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented May 29, 2018

Okay @sebmarkbage 👍

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

Loading

@bvaughn bvaughn changed the title ReactTestRenderer renders ReactDOM portals ReactTestRenderer and ReactDOM portals May 29, 2018
@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented Jun 13, 2018

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

Loading

@brokentone
Copy link

@brokentone brokentone commented Jul 19, 2018

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

Loading

@facebook-github-bot
Copy link

@facebook-github-bot facebook-github-bot commented Jul 25, 2018

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.

Loading

@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented Jul 26, 2018

Loading

@facebook-github-bot
Copy link

@facebook-github-bot facebook-github-bot commented Jul 26, 2018

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

Loading

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

@sebmarkbage sebmarkbage Aug 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

@bvaughn bvaughn force-pushed the test-renderer-portal-warning branch from e1f87ed to 8b865c3 Aug 20, 2018
@bvaughn bvaughn merged commit d670bdc into facebook:master Aug 20, 2018
1 check passed
Loading
@bvaughn bvaughn deleted the test-renderer-portal-warning branch Aug 20, 2018
@gaearon gaearon mentioned this pull request Sep 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants