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.

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

Add ES6 collection support to include() #994

Merged
merged 7 commits into from Jun 23, 2017
Merged

Add ES6 collection support to include() #994

merged 7 commits into from Jun 23, 2017

Conversation

shvaikalesh
Copy link
Contributor

Closes #970

break;

default:
var isEql = isDeep ? _.eql : function(a, b) { return a === b; };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like we should use SameValue here, but it is minor breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

Yes - we should look to change many of these instances for chai@5 i think


case 'weakmap':
case 'weakset':
if (isDeep) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@keithamus keithamus Jun 22, 2017

Choose a reason for hiding this comment

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

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

@keithamus
Copy link
Member

I'm very happy with this, but let's get another review to sign off before merge. @meeber @lucasfcosta?

var isEql = isDeep ? _.eql : function(a, b) { return a === b; };
obj.forEach(function (item) {
included = included || isEql(item, val);
});
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

@shvaikalesh
Copy link
Contributor Author

shvaikalesh commented Jun 22, 2017

One more thing: should we throw when deep.include is called on string? Currently we just ignore the flag, but such assertions do not make sense to me.

@keithamus
Copy link
Member

@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

Copy link
Contributor

@meeber meeber left a comment

Choose a reason for hiding this comment

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

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.)

@shvaikalesh
Copy link
Contributor Author

I've updated the docs and dropped WeakMap support -- we can't iterate over it's values to make an assertion.

Regarding deep flag: point raised by @meeber in #996 (comment) is valid for WeakSet targets too. I am fine with silently ignoring it.

Copy link
Contributor

@meeber meeber left a comment

Choose a reason for hiding this comment

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

LGTM!


case 'map':
var isEql = isDeep ? _.eql : SameValueZero;
obj.forEach(function (item) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lucasfcosta I think this is worth exploring. I'd prefer this kind of optimization technique be implemented in a separate PR as a refactor.

Copy link
Member

Choose a reason for hiding this comment

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

@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.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants