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.

Sign up for GitHub

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

Jump to bottom

Mounted portals show in debug and can be found #1760

Merged
merged 2 commits into from
Aug 18, 2018
Merged

Mounted portals show in debug and can be found #1760

merged 2 commits into from
Aug 18, 2018

Conversation

jgzuke
Copy link
Collaborator

@jgzuke jgzuke commented Aug 16, 2018

React 16+ adapters toTree given a HostPortal returns a portal wrapping the child tree, instead of returning the child tree directly. This lets Portals show up in debug output and be found. Added the portal wrapper with a nodeType of 'portal' and a type of react-is Portal.

@jgzuke jgzuke requested a review from ljharb August 16, 2018 22:44
jgzuke
jgzuke commented Aug 16, 2018
View reviewed changes
packages/enzyme-adapter-react-16.1/src/ReactSixteenOneAdapter.js Outdated
case HostPortal: // 4
return {
nodeType: 'portal',
type: Portal,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Im not sure if it makes more sense to use Portal here, it may be better to just use 'Portal'?

Copy link
Member

Choose a reason for hiding this comment

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

typically type is either a string (an html element) or a function (component). I think using the Portal` symbol here is perfect.

@jgzuke jgzuke force-pushed the jgzuke-support-portal-mount branch from f00cac8 to c74acb4 Compare August 16, 2018 23:00
ljharb
ljharb requested changes Aug 17, 2018
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Could we add a few tests showing that you can .find stuff inside the Portal?

packages/enzyme-adapter-react-16.1/src/ReactSixteenOneAdapter.js Outdated
case HostPortal: // 4
return {
nodeType: 'portal',
type: Portal,
Copy link
Member

Choose a reason for hiding this comment

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

typically type is either a string (an html element) or a function (component). I think using the Portal` symbol here is perfect.

packages/enzyme-adapter-react-16.1/src/ReactSixteenOneAdapter.js Outdated
return {
nodeType: 'portal',
type: Portal,
props: {},
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this shouldn't be props: { children } - ie, have the actual children provided to createPortal?

also, perhaps include the containerInfo as a "prop"?

packages/enzyme-adapter-utils/src/Utils.js
@@ -94,6 +97,10 @@ export function displayNameOfNode(node) {

if (!type) return null;

if (type === Portal) {
return 'Portal';

This comment was marked as resolved.

Sign in to view

packages/enzyme-adapter-utils/src/Utils.js
@@ -104,6 +111,9 @@ export function nodeTypeFromType(type) {
if (type && type.prototype && type.prototype.isReactComponent) {
return 'class';
}
if (type && type === Portal) {
return 'portal';

This comment was marked as resolved.

Sign in to view

packages/enzyme-test-suite/test/ReactWrapper-spec.jsx Outdated
const wrapper = mount(<Foo />);
expect(wrapper.debug()).to.equal(`<Foo>
<div>
<Portal>
Copy link
Member

Choose a reason for hiding this comment

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

perhaps this could be <Portal container={…}> - with the container display being something like containerDiv.outerHTML?

@jgzuke
Copy link
Collaborator Author

jgzuke commented Aug 17, 2018

The test for finding Portals descendants is at https://github.com/airbnb/enzyme/blob/master/packages/enzyme-test-suite/test/ReactWrapper-spec.jsx#L957 (since that behavior was working previously for mount) but I will move the new test closer.

@ljharb ljharb force-pushed the jgzuke-support-portal-mount branch from 0562916 to 605021d Compare August 18, 2018 06:37
@ljharb ljharb merged commit 3bc5de2 into enzymejs:master Aug 18, 2018
@ljharb ljharb mentioned this pull request Aug 18, 2018
Closed
4 tasks
@jgzuke jgzuke mentioned this pull request Aug 18, 2018
Merged
ljharb added a commit that referenced this pull request Aug 24, 2018
@ljharb
[enzyme-adapter-utils] v1.6.0
f9ddb18 
 - createMountWrapper: add Portals to propType (#1760, #1761, #1772)
 - add pointer events (#1753)
 - `elementToTree`: add ability to inject recursed `elementToTree` (#1760)
ljharb added a commit that referenced this pull request Aug 25, 2018
@ljharb
[enzyme-adapter-react-{16.1,16.2,16.3}] v1.1.0
7c0ce20 
 - [New] Add Portal support (#1760, #1761, #1772, #1774, @jgzuke)
 - [New] Add pointer events support (#1753, @ljharb)
 - [New] Add `displayNameOfNode`, `isValidElementType` (#1701, @jquense)
 - [New] pass the adapter into `createMountWrapper` (#1592, @jquense)
 - [Fix] `shallow`: skip updates when nextState is `null` or `undefined` (#1785, @koba04)
 - update deps
 - [meta] ensure a license and readme is present in all packages when published
ljharb added a commit that referenced this pull request Aug 25, 2018
@ljharb
[enzyme-adapter-react-16] v1.3.0
8250432 
 - [New] Add forwardRef support (#1592, @jquense)
 - [New] Add Portal support (#1760, #1761, #1772, #1774, @jgzuke)
 - [New] Add pointer events support (#1753, @ljharb)
 - [New] Add `displayNameOfNode`, `isValidElementType` (#1701, @jquense)
 - [New] pass the adapter into `createMountWrapper` (#1592, @jquense)
 - [Fix] preemptively fix compat with React v16.4.3 (#1790, #1778, @gaearon, @aweary)
 - [Fix] `shallow`: skip updates when nextState is `null` or `undefined` (#1785, @koba04)
 - update deps
 - [meta] ensure a license and readme is present in all packages when published
ljharb added a commit that referenced this pull request Aug 25, 2018
@ljharb
[enzyme] v3.5.0
2f49989 
 - [New] Add forwardRef support (#1592, @jquense)
 - [New] Add Portal support (#1760, #1761, #1772, #1774, @jgzuke)
 - [New] Add pointer events support (#1753, @ljharb)
 - [Fix] preemptively fix compat with React v16.4.3 (#1790, #1778, @gaearon, @aweary)
 - [Fix] `shallow`: prevent rerenders with PureComponents (#1786, @koba04)
 - [Fix] `shallow`: skip updates when nextState is `null` or `undefined` (#1785, @koba04)
 - [Fix] `shallow`: `setState` after `setProps` calls `componentWillReceiveProps` (#1779, @peanutenthusiast)
 - [Fix] `mount`/`shallow`: be stricter on the wrapper’s setState/setProps callback
 - [Fix] `shallow`/`mount`: improve error message when wrapping invalid elements (#1759, @jgzuke)
 - update deps
 - [Refactor] remove most uses of lodash
 - [meta] ensure a license and readme is present in all packages when published
This was referenced Aug 25, 2018
Open
Merged
Closed
Closed
This was referenced Oct 1, 2018
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb approved these changes

Assignees
No one assigned
Labels
package: enzyme-adapter-utils package: react adapter: 16 package: react adapter: 16.1 package: react adapter: 16.2 package: react adapter: 16.3
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jgzuke @ljharb