Make ReactWrapper and ShallowWrapper iterable #594
Conversation
| @@ -12,6 +12,10 @@ import { | |||
| REACT15, | |||
| } from './version'; | |||
|
|
|||
| export const ITERATOR_SYMBOL = ( | |||
| typeof Symbol === 'function' && Symbol.iterator | |||
| ) || '@@iterator'; | |||
There was a problem hiding this comment.
To avoid the complexity of having to conditionally define a method on a class when there's no Symbol.iterator available, this falls back to '@@iterator'. I don't think this should affect environments where neither is supported.
There was a problem hiding this comment.
We should absolutely not fall back to a string "@@iterator" unless it's the specific versions of Firefox where this imbues magic behavior - which can only be determined by using for..of inside a Function (so that the syntax doesn't break older engines).
|
@ljharb updated |
| @@ -102,6 +103,26 @@ export default class ShallowWrapper { | |||
| } | |||
|
|
|||
| /** | |||
| * Makes a wrapper iterable, which is useful when using destructive | |||
There was a problem hiding this comment.
@aweary forgot to remove this i think
aweary
force-pushed
the
iterable-wrapper
branch
from
fb5220f
to
3dc1d30
Compare
|
|
||
|
|
||
| if (ITERATOR_SYMBOL) { | ||
| ReactWrapper.prototype[ITERATOR_SYMBOL] = function iterator() { |
There was a problem hiding this comment.
i might be being over-pedantic here, but perhaps we should Object.defineProperty this so it's non-enumerable?
There was a problem hiding this comment.
Seems reasonable enough to me
|
LGTM, but let's get a few more eyes on it |
|
This looks good to me as well |
|
can you rebase on latest master @aweary ? |
|
@ljharb unrelated to this PR, should we remove the LGTM integration now that Github has this approvals thing? |
|
@nfcampos probably, but for now let's keep it and just make sure to use "LGTM" in our approved reviews |
| @@ -3044,4 +3044,31 @@ describeWithDOM('mount', () => { | |||
| }); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('@@iterator', () => { | |||
| it('should be iterable', () => { | |||
There was a problem hiding this comment.
these tests should be skipped when the environment lacks Symbols.
| @@ -3585,4 +3585,31 @@ describe('shallow', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| describe('@@iterator', () => { | |||
| it('should be iterable', () => { | |||
There was a problem hiding this comment.
these tests should be skipped when the environment lacks Symbols.
|
@aweary i assume you'll plan to rebase to get rid of the merge commit? :-) |
|
@ljharb of course! 👍 |
aweary
force-pushed
the
iterable-wrapper
branch
from
73ffe26
to
3cf0488
Compare
|
@aweary do you want to look into the conflicts so that this can be merged? |
aweary
force-pushed
the
iterable-wrapper
branch
from
3cf0488
to
5b9022a
Compare
|
@nfcampos rebased 👍 |
Resolves #577
cc @ljharb