Better support for iterable children #1334
Conversation
| } | ||
|
|
||
| function isIterable(obj) { | ||
| return Boolean(getIteratorFn(obj)); |
There was a problem hiding this comment.
this should use !! rather than Boolean().
| } | ||
|
|
||
| function isIterable(obj) { | ||
| return Boolean(getIteratorFn(obj)); | ||
| } | ||
|
|
||
| export function isArrayLike(obj) { | ||
| return Array.isArray(obj) || (isIterable(obj) && typeof obj !== 'string'); |
There was a problem hiding this comment.
(predates your PR, but still) let's flip the typeof check here; isIterable shouldn't be called at all in the string case.
| @@ -316,6 +316,115 @@ describe('RSTTraversal', () => { | |||
| expect(nodes).to.deep.equal([node, divA, divB, divC, divD]); | |||
| }); | |||
|
|
|||
| describe('support for arbitrary iterable children', () => { | |||
| const makeDivIterator = (lowerBound, upperBound) => { | |||
| let counter = lowerBound; | |||
There was a problem hiding this comment.
let counter = lowerBound + 'a'.charCodeAt(0); would avoid needing to look up "a"'s charCode every time
|
@ljharb, working on the feedback from your review. Thanks so much. |
|
It's ready for review again @ljharb. |
ljharb
requested review from
nfcampos,
lelandrichardson,
blainekasten,
aweary and
a team
| step = iterator.next(); | ||
| } | ||
|
|
||
| return flatArrs; |
There was a problem hiding this comment.
This feels like a lot of added complexity when flatten should almost always be called with arrays. It just seems like we're doing the same work that [...iterable] is doing, except that this approach won't benefit from any internal optimizations that the spread operator may have.
There was a problem hiding this comment.
Our use case is running our unit test suite on PhantomJS, where some of our components return Immutable List instance of React elements. As described in #1328, that feature was recently added but was not working on PhantomJS due the lack of support for Symbol.iterator. I agree the increased complexity is evident, and that's why I aimed for good test coverage. The main reason to make this change is to catch up with React, which is very flexible with the children it's able to handle, and switching from v2 to v3 may break tests of some middle to large projects using a pattern like us and results in additional effort to make the bump. Really appreciate your comments.
There was a problem hiding this comment.
@aweary, what if I add below code at the beginning of flatten function?
if (Array.isArray(arrs)) {
return arrs.reduce(
(flattened, item) => flattened.concat(isArrayLike(item) ? flatten(item) : item),
[],
);
}
That way flatten will be as performant as it used to before my change, for the most common case where arrs is a regular JS Array. But gracefully fallbacks to support any kind of iterable children React can also handle.
Let me know your thoughts. cc @ljharb
There was a problem hiding this comment.
Cool, I'm going to make that change and push.
|
@Inucoder would you please rebase this on the command line, so as to remove all merge commits? |
Inucoder
force-pushed
the
better_support_for_iterable_children
branch
from
ec131ad
to
d96e780
Compare
|
It's done @ljharb, a coworker gave me a helping hand to do the rebase. |
|
@Inucoder thanks! it looks like it's still out of date with master tho; you may need to fetch and redo the rebase? (if this gets onerous, I'm happy to take care of it for you) |
…e Object. Update isIterable function to leverage getIteratorFn. Rewrite flatten function to leverage getIteratorFn. Add unit test for flatten function. Add unit tests for arbitrary iterable children. Support for Symbol.iterator and @@iterator iterables.
Inucoder
force-pushed
the
better_support_for_iterable_children
branch
from
d96e780
to
308578a
Compare
|
Should be good now @ljharb, not a big deal. |
|
Seems good to me. Thanks for working on this. |
- [New] Better support for iterable children (enzymejs#1334)
- [New] Better support for iterable children (enzymejs#1334)
…e Object.
Update isIterable function to leverage getIteratorFn.
Rewrite flatten function to leverage getIteratorFn.
Add unit test for flatten function.
Add unit tests for arbitrary iterable children.
Support for Symbol.iterator and @@iterator iterables.