Add ES6 collection support to include() #994
Conversation
lib/chai/core/assertions.js
Outdated
| break; | ||
|
|
||
| default: | ||
| var isEql = isDeep ? _.eql : function(a, b) { return a === b; }; |
There was a problem hiding this comment.
I feel like we should use SameValue here, but it is minor breaking change.
There was a problem hiding this comment.
Yes - we should look to change many of these instances for chai@5 i think
lib/chai/core/assertions.js
Outdated
|
|
||
| case 'weakmap': | ||
| case 'weakset': | ||
| if (isDeep) { |
There was a problem hiding this comment.
I think we should consider containing code like this into a private method: it is easy to forget to process flagMsg, pass ssfi, and skip second argument.
There was a problem hiding this comment.
Agreed, but I'd like to see focus on #585 to get the assertion functions to be pure functions returning objects, so we can wrap them up in AssertionErrors
|
I'm very happy with this, but let's get another review to sign off before merge. @meeber @lucasfcosta? |
lib/chai/core/assertions.js
Outdated
| var isEql = isDeep ? _.eql : function(a, b) { return a === b; }; | ||
| obj.forEach(function (item) { | ||
| included = included || isEql(item, val); | ||
| }); |
There was a problem hiding this comment.
Can we do something here to optimize this by breaking out the loop once included is true?
I realize that then we would need to have 3 implementations, one for Array, another for Map and another for Set.
Do you think it is worth it?
There was a problem hiding this comment.
We can optimize away looping through set for non-deep include and use some + indexOf on arrays to avoid extra functions. Code:
case 'map':
let isEql = isDeep ? _.eql : function(a, b) { return a === b; };
// I believe SameValueZero should be ^^ here to be on par with Set#has
obj.forEach(function (item) {
included = included || isEql(item, val);
});
break;
case 'set':
if (isDeep) {
obj.forEach(function (item) {
included = included || _.eql(item, val);
});
} else {
included = obj.has(val);
}
break;
case 'array':
if (isDeep) {
included = obj.some(function (item) {
return _.eql(item, val);
});
} else {
included = obj.indexOf(val) !== -1;
}
break;I am not sure the benefits outweigh code amount & complexity.
EDIT: sorry, email readers.
There was a problem hiding this comment.
This looks good but I'd be a little careful here with IE11 - which has terrible support for Map/Set. It probably will work but I've often been surprised how seemingly obvious features are missing from IE11's Map/Set
|
One more thing: should we throw when |
|
@shvaikalesh I think we should - as a general future feature consideration - start adding warnings for wonky assertion chains, rather than explicitly erroring. Let's leave it out of this PR and discuss it in #996 |
There was a problem hiding this comment.
This is excellent. However, I do think it's important that the documentation for .include be updated to mention how the new types are handled (including the fact that SameValueZero equality is used).
Also, I have the same concern here as mentioned in #996 (comment). Is there any situation in which it would be reasonable for a deep flag to be applied to another assertion earlier in the chain, and thus would be safely ignored when applied to .include for weakmaps and weaksets? (I can't think of any, but want to make sure.)
|
I've updated the docs and dropped Regarding |
|
|
||
| case 'map': | ||
| var isEql = isDeep ? _.eql : SameValueZero; | ||
| obj.forEach(function (item) { |
There was a problem hiding this comment.
As @vieiralucas said, we might be able to optimize this by getting out of the loop when we found the included item.
In order to support IE and older versions of it we can throw an error inside the forEach callback and catch it.
This is a cross-platform compatible implementation I've made on Sinon for the every method. We can inspire our some implementation in that. However, even though I'd like it to be done, I won't block merging this PR because of that.
There was a problem hiding this comment.
@lucasfcosta I think this is worth exploring. I'd prefer this kind of optimization technique be implemented in a separate PR as a refactor.
There was a problem hiding this comment.
@meeber agreed.
LGTM! Let's get this merged.
| * expect(new Set([1, 2])).to.include(2); | ||
| * | ||
| * When the target is a Map, `.include` asserts that the given `val` is one of | ||
| * the values of the target. SameValueZero equality algorithm is used. |
There was a problem hiding this comment.
I think it would be good to explain how the SameValueZero algorithm works. I think many people could get confused by that and would end up either having to read the code to understand it or just avoiding using this assertion because they're not sure of what it does.
There was a problem hiding this comment.
@lucasfcosta Fair point. In this case, I would suggest moving the phrase "SameValueZero equality algorithm is used" from where it is currently down to the separate paragraph where strict and deep equality` is discussed. This would make the doc style more consistent anyway. And then rather than explain what "SameValueZero equality" is, we could provide a link to an explanation, just like we do with the "deep equal" algorithm.
There was a problem hiding this comment.
We could just refer to is as it is more commonly known; Object.is or if you'd like to we coud keep the phrasing but link to the equality article on MDN which mentions SameValueZero.
Closes #970