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

Rework deepEqual() #242

Merged
merged 3 commits into from Nov 1, 2018
Merged

Rework deepEqual() #242

merged 3 commits into from Nov 1, 2018

Conversation

@kanongil
Copy link
Member

kanongil commented Mar 7, 2018

This fixes some of the pertinent issues with deepEqual() in a performance optimized way.

Note that this intentionally does not fix #238, due to the performance loss of the fix.

Also note that I have kept the behavior regarding computed properties (see #240) for the time being, though it comes at a significant performance penalty.

@kanongil

This comment has been minimized.

Copy link
Member Author

kanongil commented Mar 7, 2018

One more important thing. I changed it to only compare own enumerable properties, rather than both enumerable, and non-enumerable. This did not cause any tests to fail.

Two reasons: It matches the Assert.deepEqual() implementation, and it can be implemented in a more performant way.

Let me know if that is an issue.

For reference: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Enumerability_and_ownership_of_properties

@nlf

This comment has been minimized.

Copy link
Member

nlf commented May 17, 2018

so this is technically a breaking change, then. right? the number of changes to the tests make me a bit uneasy, are we about to require users to change all their comparisons to pass { prototype: false }?

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented May 19, 2018

What's the story with { prototype: false }?

@kanongil

This comment has been minimized.

Copy link
Member Author

kanongil commented May 19, 2018

The { prototype: false } is mostly not required for the test itself. It's used to exercise the implementation and better catch errors if the underlying implementation changes.

Eg. Hoek.deepEqual([], {})) will find the different prototypes, and immediately return false before even calling Array.isArray(). Note that I only added it on tests that evaluates to false.

You are welcome to change it back, and see that all tests still pass but probably loose 100% test coverage.

@kanongil

This comment has been minimized.

Copy link
Member Author

kanongil commented May 19, 2018

As for whether this PR is a breaking change, it depends on if the dropped comparison of non-enumerable properties is considered a bugfix. FYI, I don't believe it changes anything for Hapi.

One point, looking at Assert.deepStrictEqual(), is that it needs some more code to handle the name and message of Error's.

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented May 21, 2018

WFM then.

@kanongil

This comment has been minimized.

Copy link
Member Author

kanongil commented May 23, 2018

If this is merged as a breaking change, can we have another look at #240? FYI, it is simple to remove computed property support from the patch, if you agree it is better without.

I also had a look at where it is used in the hapijs org, and outside testing (through Code), I could only find one use here: Joi:lib/types/array/index.js#L507.

@kanongil

This comment has been minimized.

Copy link
Member Author

kanongil commented May 29, 2018

I removed the "compare computed property without calling the getters" support. This was not done correctly anyway (in the existing implementation as well). It could still be called on the ref, if the property in obj didn't have a getter.

@kanongil

This comment has been minimized.

Copy link
Member Author

kanongil commented Jun 19, 2018

This patch also fixes #260. Including a test.

@kanongil kanongil force-pushed the kanongil:deepequal-refactor branch from cf9d510 to 9d6b25e Aug 12, 2018
@hueniverse hueniverse self-assigned this Nov 1, 2018
@hueniverse hueniverse added this to the 6.0.0 milestone Nov 1, 2018
@hueniverse hueniverse merged commit a3971b1 into hapijs:master Nov 1, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.