Support array-like iterable children in v3 #1187

kmcq · 2017-09-28T02:16:21Z

What this is

This adds support for non-Array, but still iterable, children, such as Immutable-js Lists, Sets, etc., which is supported by React:

const Component = () => {
  const listOfThings = Immutable.List([1, 2, 3]);
  return <div>{listOfThings.map(thing => <span>{thing}</span>)}</div>;
};

The React PR that originally added this functionality is here: facebook/react#2376.

The code

Check to see if children is iterable, and if so, use Array.from to convert it to an array.

I like this approach because it compartmentalizes the knowledge that children are sometimes things other than arrays.

kmcq · 2017-09-28T02:18:58Z

packages/enzyme-adapter-utils/src/elementToTree.js

@@ -0,0 +1,65 @@
+const ITERATOR_SYMBOL = typeof Symbol === 'function' && Symbol.iterator;


I saw that enzyme has this elsewhere, but in a different package. If there's a good way to share this across lerna packages, please let me know. I haven't used lerna before 😄

kmcq · 2017-09-28T02:19:31Z

packages/enzyme-adapter-utils/src/Utils.js

@@ -81,43 +88,6 @@ export function assertDomAvailable(feature) {
  }
 }

-export function nodeTypeFromType(type) {


Moved this to its own file so that elementToTree could still use it

kmcq · 2017-09-28T03:10:21Z

After searching around the codebase some more, I see that the elementToTree that I patched is not the only elementToTree (v13 has its own) and also there are potentially other places that would have to change for this to work, for instance childrenToArray in enzyme/Utils. I'm also not convinced that the tests I wrote are the best way to test that this works (I left them failing).

I'm going to pause on this for now to see if any maintainers have ideas about how best to tackle this.

lelandrichardson · 2017-09-30T17:19:30Z

Interesting! I didn't know React had this capability.

kmcq · 2017-10-05T17:27:25Z

fyi I plan on coming back to this next week!

kmcq · 2017-10-11T03:22:18Z

@lelandrichardson I have changed this to now be about adding Immutable.js support. Please see the updated description and code and let me know if you have any questions!

ljharb

Generic iterables makes sense to me; that's in the language.

A .toArray function is arbitrary and non-standard; unless React supports this explicitly, I do not think we should support it in enzyme.

kmcq · 2017-10-11T13:30:31Z

Hi @ljharb thanks for the review. I have made this more generic -- I did some more research and found out that all iterables support Array.from. This seems more in line with the language spec. Please read again and let me know :)

ljharb

Thanks, this looks much better!

ljharb · 2017-10-11T19:37:49Z

packages/enzyme-adapter-react-13/src/ReactThirteenElementToTree.js

@@ -23,6 +24,8 @@ export default function elementToTree(el) {
  let rendered = null;
  if (Array.isArray(children)) {
    rendered = flatten(children, true).map(elementToTree);
+  } else if (isIterable(children) && typeof children !== 'string') {
+    rendered = flatten(Array.from(children), true).map(elementToTree);


since we use babel, this could also be flatten([...children], true)

ljharb · 2017-10-11T19:38:14Z

packages/enzyme-adapter-utils/src/Utils.js

+  return (
+    typeof Symbol === 'function' &&
+    typeof Symbol.iterator === 'symbol' &&
+    obj != null &&


i'd probably move the obj check higher, since that's faster then checking Symbols

Awesome, I wasn't aware of that! Thanks!

ljharb · 2017-10-11T19:38:27Z

packages/enzyme-adapter-utils/src/Utils.js

@@ -104,6 +113,8 @@ export function elementToTree(el) {
  let rendered = null;
  if (Array.isArray(children)) {
    rendered = flatten(children, true).map(elementToTree);
+  } else if (isIterable(children) && typeof children !== 'string') {
+    rendered = flatten(Array.from(children), true).map(elementToTree);


similarly, flatten([...children], true)

ljharb · 2017-10-11T19:39:28Z

packages/enzyme-test-suite/test/RSTTraversal-spec.jsx

+        </div>,
+      );
+      treeForEach(node, spy);
+      expect(spy.callCount).to.equal(3);


Could we also use expect(spy.calls).to.deep.equal in both places, and assert that it's called with the right items?

It'd also be great to test with a Map as well as a Set

Yes for sure, I like that. The tree objects are pretty complex, how do you feel about just checking part of it, say keys? Like this:

it('should handle Map children', () => { const spy = sinon.spy(); const twoDivMap = new Map([ [<div key="a" />], [<div key="b" />], ]); const node = $( <div key="root"> {twoDivMap} </div>, ); treeForEach(node, spy); expect(spy.callCount).to.equal(3); const keys = spy.args.map(arg => arg[0].key); expect(keys).to.deep.equal(['root', 'a', 'b']); });

ah nevermind, even better I can just use $ and check the whole thing:

it('should handle Map children', () => { const spy = sinon.spy(); const twoDivMap = new Map([ [<div key="a" />], [<div key="b" />], ]); const divA = $(<div key="a" />); const divB = $(<div key="b" />); const node = $( <div key="root"> {twoDivMap} </div>, ); treeForEach(node, spy); expect(spy.callCount).to.equal(3); const nodes = spy.args.map(arg => arg[0]); expect(nodes).to.deep.equal([node, divA, divB]); });

I'll get this committed soon!

kmcq · 2017-10-11T21:04:45Z

Alrighty! @ljharb travis is happy and I think I answered your comments. Can you please review again?

ljharb

Looks great! Although it does seem like you've only added iterable support in the react 13 adapter - the 14, 15, 15.4, and 16 adapters would need it as well.

kmcq · 2017-10-11T22:35:47Z

Thanks! Actually the v13 adapter is special-cased. The others all use the enzyme-adapter-utils version of elementToTree, which is part of this!

ljharb

Awesome, thanks for clarifying :-)

stevenmusumeche · 2017-10-21T15:12:26Z

Any idea when this might be released?

ljharb · 2017-10-21T15:38:54Z

@stevenmusumeche when a few more collaborators have reviewed it.

kmcq · 2017-10-27T21:07:38Z

would it be helpful if I periodically rebase this? otherwise I can just do it once we're ready

stevenmusumeche · 2017-10-27T21:42:05Z

I hate to be a pest, but is there a way to move the review up in priority for the other collaborators? The code and tests look great to me, but I am not intimately familiar with the codebase.

It is something that worked before in v2 and doesn't in v3 and is preventing quite a few companies from upgrading to React 16.

aweary

Sorry it took so long! I'm taking some time away from OSS right now. This looks good though 👍

ljharb · 2017-10-27T21:49:33Z

@kmcq please do rebase it, yes :-)

kmcq · 2017-10-27T21:58:32Z

rebased!

stevenmusumeche · 2017-10-27T23:56:48Z

Thank you all so much for what you do.

kmcq · 2017-10-28T00:04:24Z

Thanks @ljharb!

I appreciate everyone's reviews. This was my first time contributing to a library like this and you all made it fun and I learned some things too. Cheers!

kamui · 2017-10-28T18:55:12Z

Thanks @kmcq!

Any ETA on when a new version will be cut? This was the only thing blocking us from upgrading to react 16. Thanks!

Inucoder · 2017-11-01T17:07:17Z

Looking forward for this to be released! Great work guys!

ljharb · 2017-11-01T17:13:08Z

@kamui no ETA yet. I'm shooting for this week, ASAP.

ljharb · 2017-11-02T04:02:45Z

All adapters, as well as enzyme-adapter-utils, have been bumped.

kmcq · 2017-11-02T14:01:34Z

Hey @ljharb, sorry to bug you but it looks like the React 15 adapter's release didn't make it to NPM: https://www.npmjs.com/package/enzyme-adapter-react-15. I checked and the rest look good 👍 Is that something that you need to handle or is that one just lagging behind? Thanks!

Edit: I should point out that this is not a big issue, nor should it block anyone, since the change was in enzyme-adapter-utils@1.1.0, which that above adapter lists as having a dependency of ^1.0.0. The old adapter will pull in the new util package.

Edit2: It's there now! Thanks so much!

Inucoder · 2017-11-03T20:34:29Z

Hey guys, I'm running enzyme 3.1.0, enzyme-adapter-react-15 1.0.4, enzyme-adapter-utils 1.1.0 along with Karma.

'use strict';

import Enzyme          from 'enzyme';
import Immutable     from 'immutable';
import Adapter         from 'enzyme-adapter-react-15';
import {isArrayLike} from 'enzyme-adapter-utils';

Enzyme.configure({adapter: new Adapter()});

describe('Enzyme 3', () => {
  it('should play nice with Immutable.List children', () => {
    const children = Immutable.fromJS([1, 2, 3]);

    expect(isArrayLike(children))
      .toBe(true);
  });
});

Above unit test doesn't pass, since isArrayLike doesn't think Immutable.List is array-like, to be more specific isIterable from enzyme-adapter-utils returns false for an Immutable List instance. I also can see you are testing against ES6 Map and Set in your test suite, but support for Immutable children is missing.

I could add Immutable as a dependency of enzyme-adapter-utils to extend isArrayLike for this use case. Or if you think about a better way to achieve it, I could give it a shot, add some tests and submit a PR.

I think having support for Immutable children makes sense because:

Was supported in v2.
It's supported by React itself.

I'm looking forward to collaborate, keep the great work!

ljharb · 2017-11-03T22:59:28Z

@kcmq thanks, your comment reminded me to publish it :-) all set.

@Inucoder Immutable.List should be an iterable, even if not array-like - so I'd expect it should work. Are you using the latest version of immutable?

Inucoder · 2017-11-03T23:37:39Z

@ljharb, the test I shared was with Immutable 3.8.2, which is the latest stable release. Just pulled 4.0.0-rc.9 and test still not passing. I can add support for it based on how React checks for it facebook/react@c07ea0b. Let me know your thoughts.

Inucoder · 2017-11-03T23:57:36Z

@ljharb, I found that the issue was because we are running our unit tests on PhantomJS, and ES6 Symbol API is missing there, and might be the case for old browsers. Tried to run the test on recent Chrome version and works just fine.

We could change

function isIterable(obj) {
  return (
    obj != null &&
    typeof Symbol === 'function' &&
    typeof Symbol.iterator === 'symbol' &&
    typeof obj[Symbol.iterator] === 'function'
  );
}

in the enzyme-adapter-utils, to

function isIterable(obj) {
  return (
    obj != null && (
      (
        typeof Symbol === 'function' &&
        typeof Symbol.iterator === 'symbol' &&
        typeof obj[Symbol.iterator] === 'function'
      ) ||
      obj['@@iterator']
    )
  );
}

that's exactly what React does.

ljharb · 2017-11-04T02:08:23Z

Hmm - @@iterator is a fallback that only a few versions of Firefox officially supported; I'm hesitant to support it in enzyme; although since React unfortunately chose to support it, we might have to.

Can you confirm whether React supported this behavior down to 0.13? Also, can you file a separate issue about this?

Inucoder · 2017-11-05T01:48:38Z

For sure @ljharb, I'll take a look into that.

ljharb · 2017-12-22T19:37:03Z

Linking to #1334 as well.

kmcq commented Sep 28, 2017

View reviewed changes

kmcq force-pushed the iterable-children branch from 05f921f to 2e1b7b8 Compare September 28, 2017 03:01

kmcq changed the title ~~Allow for non-Array, but Iterable, children~~ [WIP] Allow for non-Array, but Iterable, children Sep 28, 2017

kmcq changed the title ~~[WIP] Allow for non-Array, but Iterable, children~~ Support Immutable.js array-like children in v3 Oct 11, 2017

kmcq force-pushed the iterable-children branch from ad838e4 to 8f952c4 Compare October 11, 2017 03:30

ljharb requested changes Oct 11, 2017

View reviewed changes

kmcq force-pushed the iterable-children branch from 8f952c4 to 4475fbe Compare October 11, 2017 13:20

kmcq changed the title ~~Support Immutable.js array-like children in v3~~ Support array-like iterable children in v3 Oct 11, 2017

ljharb requested changes Oct 11, 2017

View reviewed changes

kmcq force-pushed the iterable-children branch 2 times, most recently from 2c90486 to 2d43e65 Compare October 11, 2017 20:55

ljharb reviewed Oct 11, 2017

View reviewed changes

ljharb added semver: minor New stuff. API: mount package: react adapter: 0.13 API: shallow package: react adapter: 0.14 package: react adapter: 15 package: react adapter: 15.4 package: react adapter: 16 labels Oct 11, 2017

ljharb approved these changes Oct 11, 2017

View reviewed changes

ljharb requested a review from lelandrichardson October 11, 2017 22:46

aweary approved these changes Oct 27, 2017

View reviewed changes

kmcq added 4 commits October 27, 2017 17:57

Allow for non-Array, but Iterable, children

0fcd897

Explictly add support for Immutable instead of generic Iterables

d432c69

Support iterables generically

321c807

Add support for sibling iterables

3ad399c

kmcq force-pushed the iterable-children branch from 703eb0a to 3ad399c Compare October 27, 2017 21:57

ljharb approved these changes Oct 27, 2017

View reviewed changes

ljharb merged commit a1d076d into enzymejs:master Oct 27, 2017

kmcq deleted the iterable-children branch October 28, 2017 00:02

Inucoder mentioned this pull request Nov 6, 2017

Iterable children not working on environments where Symbol API is not available #1328

Closed

		@@ -0,0 +1,65 @@
		const ITERATOR_SYMBOL = typeof Symbol === 'function' && Symbol.iterator;

Support array-like iterable children in v3 #1187

Support array-like iterable children in v3 #1187

Conversation

kmcq commented Sep 28, 2017 • edited Loading

What this is

The code

kmcq Sep 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmcq commented Sep 28, 2017 • edited Loading

lelandrichardson commented Sep 30, 2017

kmcq commented Oct 5, 2017

kmcq commented Oct 11, 2017 • edited Loading

ljharb left a comment

Choose a reason for hiding this comment

kmcq commented Oct 11, 2017

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmcq commented Oct 11, 2017

ljharb left a comment

Choose a reason for hiding this comment

kmcq commented Oct 11, 2017 • edited Loading

ljharb left a comment

Choose a reason for hiding this comment

stevenmusumeche commented Oct 21, 2017

ljharb commented Oct 21, 2017

kmcq commented Oct 27, 2017

stevenmusumeche commented Oct 27, 2017

aweary left a comment

Choose a reason for hiding this comment

ljharb commented Oct 27, 2017 • edited Loading

kmcq commented Oct 27, 2017

stevenmusumeche commented Oct 27, 2017

kmcq commented Oct 28, 2017

kamui commented Oct 28, 2017

Inucoder commented Nov 1, 2017

ljharb commented Nov 1, 2017

ljharb commented Nov 2, 2017

kmcq commented Nov 2, 2017 • edited Loading

Inucoder commented Nov 3, 2017

ljharb commented Nov 3, 2017

Inucoder commented Nov 3, 2017

Inucoder commented Nov 3, 2017 • edited Loading

ljharb commented Nov 4, 2017

Inucoder commented Nov 5, 2017

ljharb commented Dec 22, 2017

kmcq commented Sep 28, 2017 •

edited

Loading

kmcq Sep 28, 2017 •

edited

Loading

kmcq commented Sep 28, 2017 •

edited

Loading

kmcq commented Oct 11, 2017 •

edited

Loading

kmcq commented Oct 11, 2017 •

edited

Loading

ljharb commented Oct 27, 2017 •

edited

Loading

kmcq commented Nov 2, 2017 •

edited

Loading

Inucoder commented Nov 3, 2017 •

edited

Loading