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

Fix ShallowWrapper for array-rendering components #1498

Merged
merged 1 commit into from
Jan 30, 2018
Merged

Fix ShallowWrapper for array-rendering components #1498

merged 1 commit into from
Jan 30, 2018

Conversation

vsiao
Copy link
Contributor

@vsiao vsiao commented Jan 27, 2018

This PR is in support of #1149.
There are two main assumptions that I've fixed:

  • in ReactSixteenAdapter, flatten and map over the render output if possible, instead of assuming that it is always an element
  • in ShallowWrapper, reuse existing logic to set NODE and NODES appropriately when the root node is an array

ljharb
ljharb requested changes Jan 27, 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.

Thanks!

Could we also take care of mount while we're at it?

packages/enzyme/src/ShallowWrapper.js Outdated
privateSet(this, NODE, getRootNode(this[RENDERER].getNode()));
privateSet(this, NODES, [this[NODE]]);
this.length = 1;
this.setNodesInternal(getRootNode(this[RENDERER].getNode()));
Copy link
Member

Choose a reason for hiding this comment

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

this makes setNodesInternal a public function; it's important to minimize the publicly observable API. Can this be moved to a module-scoped function?

@vsiao
Copy link
Contributor Author

vsiao commented Jan 27, 2018

I applied the same module-scoped NODES-setter helper to ReactWrapper and added a basic compatibility test to ReactWrapper-spec, but I don't know if that counts as "taking care" of mount. The test I wrote would've passed even before my changes.

There are deeper problems associated with achieving full support for array-rendering that I'd rather not get into right now—primarily in that ReactDOM.findDOMNode(instance) will return the first DOM node rendered by instance and not a full array. So I imagine one of two things has to happen in order to move forward: a) React exposes an API to get a list of DOM nodes rendered by an instance, or b) Enzyme stops using findDOMNode. Until then, any ReactWrapper method that uses adapter.nodeToHostNode() will be broken when applied to a component that returns an array.

@ljharb
Copy link
Member

ljharb commented Jan 27, 2018

Hmm. Is there any way inside the React 16 adapter, in findDOMNode, to determine when this bug is happening?

Separately, could the adapter keep track of which components returned an array or fragment, and then inside findDOMNode, know to return an array of nodes?

@vsiao
Copy link
Contributor Author

vsiao commented Jan 27, 2018

Sorry, I can't commit time to exploring that. I don't think it's a great idea to do it in this pull request anyway, given that this is nicely self-contained and a strict improvement over what was there before.

@vsiao
Copy link
Contributor Author

vsiao commented Jan 30, 2018

@ljharb is there anything else I can do to get this merged?

@ljharb ljharb force-pushed the array-render-fix branch 2 times, most recently from 2385783 to 6f7f4fb Compare January 30, 2018 07:21
@ljharb ljharb merged commit 6f7f4fb into enzymejs:master Jan 30, 2018
@vsiao vsiao deleted the array-render-fix branch February 1, 2018 10:07
@jorgefilipecosta jorgefilipecosta mentioned this pull request Feb 28, 2018
Merged
@louisscruz
Copy link
Contributor

@ljharb Is there a plan to publish this soon? I currently have enzyme-adapter-react-16 version 1.1.1 installed, but this change doesn't come along with it. I'm still seeing this in my node_modules/enzyme-adapter-react-16/src/ReactSixteenAdapter.js:

 rendered: elementToTree(output),

@ljharb
Copy link
Member

ljharb commented Apr 3, 2018

I'm hoping to get a number of changes in the next release related to fragments/arrays; I'll take a look at publishing soon.

ljharb added a commit that referenced this pull request Aug 8, 2018
@ljharb
[enzyme-adapter-react-16] v1.2.0
f5a648c 
 - [new] add `isFragment` (#1733)
 - [new] Add `displayNameOfNode`, `isValidElementType`
 - [new] `mount`: add `hydrateIn` option (#1317, #1707)
 - [new] Add support for react context element types (#1513)
 - [new] `shallow`: Add getSnapshotBeforeUpdate support (#1657)
 - [fix] portals and roots may render fragments (#1733)
 - [fix] add missing support for animation events (#1569)
 - [fix] `shallow`: SFCs do not get a `this` (#1703)
 - [refactor]: add “lifecycles” adapter option (#1696)
 - [fix] call ref for a root element (#1541)
 - [fix] Allow empty strings as key props (#1524)
 - [fix] Fix ShallowWrapper for array-rendering components (#1498)
 - [refactor] use `react-is` package
 - [meta] ensure a license and readme is present in all packages when published
@ljharb
Copy link
Member

ljharb commented Aug 8, 2018

The following packages are now released:

This was referenced Aug 8, 2018
Open
Merged
Closed
@ljharb ljharb mentioned this pull request Aug 11, 2018
Open
41 tasks
@dependencies dependencies bot mentioned this pull request Aug 13, 2018
Closed
sethbattin added a commit to sethbattin/gluestick that referenced this pull request Sep 28, 2018
@sethbattin
update enzyme-react-adapter to 1.5.0
c1d66ff 
This supports shallow rendering and `.find` of `React.Fragment`.  See enzymejs/enzyme#1498
@sethbattin sethbattin mentioned this pull request Sep 28, 2018
Merged
christinebrass pushed a commit to TrueCar/gluestick that referenced this pull request Sep 28, 2018
@sethbattin @christinebrass
update enzyme-react-adapter to 1.5.0 (#1210)
25b8975 
This supports shallow rendering and `.find` of `React.Fragment`.  See enzymejs/enzyme#1498
This was referenced Oct 1, 2018
Open
Closed
@ljharb ljharb mentioned this pull request Oct 26, 2018
Closed
13 tasks
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
API: shallow Fiber Support package: react adapter: 16 semver: patch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vsiao @ljharb @louisscruz