Refactor @wry/equality to support custom [deepEquals] methods#230
Open
Refactor @wry/equality to support custom [deepEquals] methods#230
@wry/equality to support custom [deepEquals] methods#230Conversation
benjamn
commented
Sep 15, 2021
packages/equality/src/equality.ts
Outdated
Comment on lines
+6
to
+7
| export function equal(a: any, b: any): boolean { | ||
| try { | ||
| return check(a, b); | ||
| } finally { | ||
| previousComparisons.clear(); | ||
| } | ||
| return new DeepChecker().check(a, b); |
Owner
Author
There was a problem hiding this comment.
For better or worse, since equality checking now potentially calls into user-provided deepEquals methods, we cannot be sure the check(a, b) function will return before other code calls equal(...), so the trick of using a single previousComparisons map and clearing it after each check no longer works, and we need to allocate a separate DeepChecker (with its own comparisons map) for each call to equal(a, b).
benjamn
added a commit
that referenced
this pull request
Sep 15, 2021
The `@wry/*` packages are all believed to work in Node 10, but that Node.js version has been end-of-life'd (even for security updates) since April 2021: https://endoflife.date/nodejs Removing v10 should fix the tests in PR #230, which are broken because Node 10 does not understand class property syntax in the `tests.cjs.js` bundle. The main `@wry/equality` library continues to be compiled to es2015, eliminating class syntax, but the tests need to be compiled to esnext to test generator and async function equality. There may be a way to satisfy both of these constraints, but the easiest solution right now is to avoid testing in Node 10.
benjamn
commented
Sep 15, 2021
We can't get away with having only one previousComparisons Map any more, now that we're allowing user-provided code to run during the recursive comparison, because that user-provided code could call the top-level equal(a, b) function reentrantly.
Since array equality checking no longer falls through to the object case, we can preserve the `definedKeys` behavior for objects (introduced in #21) for arrays, by treating any array holes as undefined elements, using an ordinary `for` loop. Using `a.every` doesn't work because `Array` iteration methods like `Array.prototyp.every` skip over holes.
The `@wry/*` packages are all believed to work in Node 10, but that Node.js version has been end-of-life'd (even for security updates) since April 2021: https://endoflife.date/nodejs Removing v10 should fix the tests in PR #230, which are broken because Node 10 does not understand class property syntax in the `tests.cjs.js` bundle. The main `@wry/equality` library continues to be compiled to es2015, eliminating class syntax, but the tests need to be compiled to esnext to test generator and async function equality. There may be a way to satisfy both of these constraints, but the easiest solution right now is to avoid testing in Node 10.
Using a Symbol should remove any uncertainty about whether the object in question truly intended to implement the Equatable interface, or just happens to define a method called "deepEquals", which might or might not have the same signature.
93d405a to
e87f5e3
Compare
@wry/equality to support custom deepEquals methods@wry/equality to support custom [deepEquals] methods
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
@wry/equalitypackage currently uses only===equality to compare objects whoseObject.prototype.toStringtag is something other than[object Object], but it can mistakenly use deep equality for custom classes/objects that use the default[object Object]tag. This is a mistake because there's no guarantee iterating over the public enumerable properties of an arbitrary object is a good way to check for equality, so it's better to stay safe and use===.This PR fixes that mistake by comparing objects that have custom prototypes (other than
Object.prototypeornull) using only===by default. Since this restriction potentially leaves custom objects with no way to participate in deep equality checking, this PR allows objects to implement a[deepEquals]method if they want to participate, wheredeepEqualsis aSymbolthat must be imported from the@wry/equalitypackage:This optional
[deepEquals]method must be defined by both objects, anda[deepEquals](b)must agree withb[deepEquals](a)(ifa[deepEquals] !== b[deepEquals]). If you need to perform nested comparisons, you should use the predicate function passed as the second parameter (equalin thePoint2Dexample above), since it knows which objects have been compared previously, so cycles in the graph will not cause infinite loops.In the process of supporting
[deepEquals], I realized the bigswitch (aTag) {...}list within@wry/equalityhad gotten long enough to be slower than using aMapto look up checker functions (which should take constant time, regardless of how many types are supported by@wry/equality), so I refactored that system to use aMapinstead of aswitch.