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

fix: check array's custom properties #38

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lucasfcosta
Copy link
Member

@lucasfcosta lucasfcosta commented Jun 1, 2017

Hi, folks!

While working on sinonjs/sinon#1322 I've found out that we're not checking for custom properties in arrays when doing deep equality on them.

While this might be considered a bad practice, it is indeed possible and more common that we'd like it to be, so this fix may come in handy.

Before this commit, two different arrays, like these ones, would yield false positives:

var arrOne = [1, 2, 3]
arrOne.foo = 'bar'

var arrTwo = [1, 2, 3]
arrTwo.foo = 'baz'

deepEqual(arrOne, arrTwo) // true

Also, I didn't use getEnumerableKeys when getting the Array's properties because we don't want to check the Array prototype, because when dealing with Buffers or TypedArrays, they might have different offsets due to the fact of being binary data and this could cause problems. Especially when using Buffer itself.

If you don't mind I could add a check to avoid comparing the offset property on TypedArrays only so that we can compare the other properties in the array prototype, but I don't that would be productive.

I also added tests for this behavior.

@keithamus
Copy link
Member

keithamus commented Jun 1, 2017

I'm on the fence about this, but regardless of those opinions, I would say this is probably a feat and I feel like its a BREAKING CHANGE as there is no documentation to suggest this would work but doesn't.

The code itself is fine, again regardless of my fence-sitting. But I would also say that if this is true:

Also, I didn't use getEnumerableKeys when getting the Array's properties because we don't want to check the Array prototype, because when dealing with Buffers or TypedArrays, they might have different offsets due to the fact of being binary data and this could cause problems. Especially when using Buffer itself.

Then we should write tests that validate against these cases to ensure that later on a PR is not made to optimise this path and use getEnumerableKeys


Sidebar: Also, looks like node 0.12 is failing, but as it is EOL maybe this PR (especially if it is a breaking change) could drop Node 0.12 support?

@shvaikalesh
Copy link
Contributor

Hey @lucasfcosta, great job!

The most common case of having to deal with non-index keys is result of RegExp#exec.

Please, consider

/a/.exec("a").should.eql(["a"])

If I read correctly, PR makes that throw. Is this something we are OK with? It is impossible to special-case exec's result without patching it (pretty neat in ES6, though).

@lucasfcosta
Copy link
Member Author

lucasfcosta commented Jun 1, 2017

@keithamus Thanks for your feedback 😄.

We already have tests that deal with this. The Buffer one is an example. Even though two buffers are equal, they have different offset properties and thus this will cause this test to fail.

IMO we should indeed drop v0.12. I've just ammended a change to my commit for that.

@shvaikalesh that's a good question. I'd like it to pass, but I think it's more adequate to let people deal with this by adding the missing properties to the expected result than to throw incorrectly on the other cases, after all, this is how the real result of running exec looks like.

return false;
}

for (var prop in rightHandProps) {
Copy link
Contributor

@shvaikalesh shvaikalesh Jun 2, 2017

Choose a reason for hiding this comment

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

Maybe we can do

return rightHandProps.every(function(prop) {
  return deepEqual(leftHandOperand[prop], rightHandOperand[prop], options);
});

to avoid for in

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll try this and run the benchmarks to see if it's worth it, but I do agree it would be a lot more clean.

Choose a reason for hiding this comment

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

Using for ... in the way it is used here would actually open up another bug as it would also test for inherited properties.
The fasted way to check this properly is to use Object.keys with a plain for loop.

@meeber
Copy link
Contributor

meeber commented Aug 5, 2017

@lucasfcosta I'm late to the party but this change makes sense to me. Were you able to compare the performance of .every vs for in?

@lucasfcosta
Copy link
Member Author

Hi @meeber, unfortunately I didn't. I've been very busy in the last few weeks because I'll be moving to London in the next two months so I'm kind of getting things done around here in Brazil so that I can be ready to move ASAP. So I'd like to say sorry for not being able to be as present as I've been before. Hopefully I'll have some free time this weekend so that I can check this and finish this PR and get back to contributing more frequently as soon as things are set up.

Btw, thanks everyone for keeping things going, I've seen you've done some amazing work recently.

@meeber
Copy link
Contributor

meeber commented Aug 8, 2017

@lucasfcosta Sounds exciting! Congrats on the big move! Take your time with things, Chai will still be here when you're ready.

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

Successfully merging this pull request may close these issues.

5 participants