-
Notifications
You must be signed in to change notification settings - Fork 20
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
Allow comparing special/tagged objects #120
base: master
Are you sure you want to change the base?
Conversation
43eecc0
to
21d8174
Compare
Apologies for not getting to this sooner, this flew under my radar. I like the general concept; I was unaware that setting a custom |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the comments here are to stimulate discussion, but I'd prefer to flesh them out before approving.
@@ -25,6 +25,7 @@ const STANDARD_COMPARATOR_OPTIONS = { | |||
areRegExpsEqual, | |||
areSetsEqual, | |||
areTypedArraysEqual, | |||
unknownTagComparators: {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation: Change to a shared callback areUnhandledObjectsEqual
which handles all unhandled tags.
In the description you mention that you wanted to go with an object of keys to avoid potential refactor pitfalls with version changes, but in reality the refactor work would still be the same. There really isn't a scale issue here since practical usage doesn't involve having a ton of these custom tags for comparison, and it would also allow for an escape hatch if someone wanted to include known-but-unhandled object types in their comparison, such as Promise
, WeakMap
/ WeakSet
, etc.
@@ -38,6 +38,7 @@ const { assign } = Object; | |||
const getTag = Object.prototype.toString.call.bind( | |||
Object.prototype.toString, | |||
) as (a: object) => string; | |||
const getShortTag = (a: object): string | undefined => a != null ? (a as any)[Symbol.toStringTag] : undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: Since the use of Symbol.toStringTag
updates the output of Object.prototype.toString.call()
, it can be used directly.
@@ -194,7 +196,8 @@ export function createEqualityComparator<Meta>({ | |||
// equality is (`Error`, etc.), the subjective decision is to be conservative and strictly compare. | |||
// In all cases, these decisions should be reevaluated based on changes to the language and | |||
// common development practices. | |||
return false; | |||
const unknownTagComparator = unknownTagComparators[getShortTag(a) ?? tag]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Request: I know this is a bit more concise, but I request that the explicit if
check is done before this.
if (unknownTagComparator) {
return unknownTagComparator(a, b, state);
}
// Massive comment explaining ultimate fallback
...
return false;
It keeps the code comments applicable, and also allows for smaller diffs in the future if it was preferred to be bumped in priority instead of the ultimate fallback.
I wanted to add a custom comparator to handle
Temporal.Instant
for https://fullcalendar.io/docs#toc to callequals
but was surprised thatareObjectsEqual
was never called. The reason is that the TC39 proposal requires[Symbol.toStringTag]
to be set, which in turn makesfast-equals
to not call any comparator but instead returnfalse
without any way to override this behavior.I initially thought about adding a
areUnknownTaggedObjectsEqual
callback instead, but I think this would make it harder to do non-breaking additions in the future. For example if Temporal gets approved and this library wants to add support for it directly, one can't warn/fallback to the user's behavior if they defined it. So instead the config allows passing a tag-comparator mapping.Let me know if you'd rather want to go a different route.