You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
And set a breakpoint in the expect-utilseq function.
Expected behavior
I expected toEquals and eq to use the SERIALIZABLE_PROPERTIES for the equality checked, but it is only checked for the diff formatting.
Actual behavior
SERIALIZABLE_PROPERTIES is actually only checked in the printDiffOrStringify method.
Additional context
I work with an ORM that creates deeply-interlinked entities, and so have been wanting to customize the toEqual functionality for awhile now--particularly our diffs toEqual failures can get huge.
The new SERIALIZABLE_PROPERTIES looked really neat, but I was surprised to see it's only used by the diff printer, and not the core eq logic itself.
I admittedly do not have a bug reproducing exactly why that's problematic, but it does just seem unintuitive? Either from two perspectives:
The name SERIALIZABLE_PROPERTIES to me infers it will be used "for serialization" which I would have expected to apply to both "the toEqual check" as well as "the toEqual diff", but also
If the core expect-utilseq logic did not need SERIALIZABLE_PROPERTIES to avoid blowing up on the getter from the original PR's use case (which FWIW we've definitely hit that bug too, that's why I care about this), then why did matcher-utils need a new custom property? Couldn't matcher-utils have just used expect-utils logic?
I guess I don't have a true bug per se, so happy to close this out @georgekaran / @SimenB , but it just seems a little disjoint, i.e.:
custom equality checkers are hook into toEqualeq but not the diff creation
SERIALIZABLE_PROPERTIES hooks into the diff creation, but not toEqualeq
I totally get evolving ubiquitously-used software over-time is basically impossible 😓 , but just seems like there is a lot of disjointness, and maybe SERIALIZE_PROPERTIES as a user-observable flag wasn't a necessary addition, if matcher-utils-the-diff and expect-utils-the-equality reused more of their core functionality.
Again, not a bug per se, just an observation that SERIALIZABLE_PROPERTIES doesn't work the way I expected, and I think for our ORM usage we'll either have to do both custom equality checker + SERIALIZEABLE_PROPERTIES to get toEqual to do what we want. Which is fine too.
Thanks!
Environment
Jest v30.0.0-alpha.4
The text was updated successfully, but these errors were encountered:
Your points make a lot of sense when it comes to the SERIALIZABLE_PROPERTIES. To be honest, maybe the name SERIALIZABLE_PROPERTIES wasn't the best choice 😅, which might have led to some confusion as it seems to imply a broader scope than it currently has.
That being said, I think we could go to either two directions:
Rename this property to make it clear that is only used by the diff printer;
Apply this same variable to equality checkers.
Personally the second option makes more sense to me, but is probably gonna take a lib bit more time than just a renaming.
Let me know what you guys think, so I can start working on a draft to address this issue and aim to include it in a future pull request.
Version
30.0.0-alpha.4
Steps to reproduce
Setup a test with the new SERIALIZABLE_PROPERTIES feature:
And set a breakpoint in the
expect-utils
eq
function.Expected behavior
I expected
toEquals
andeq
to use theSERIALIZABLE_PROPERTIES
for the equality checked, but it is only checked for the diff formatting.Actual behavior
SERIALIZABLE_PROPERTIES
is actually only checked in theprintDiffOrStringify
method.Additional context
I work with an ORM that creates deeply-interlinked entities, and so have been wanting to customize the
toEqual
functionality for awhile now--particularly our diffstoEqual
failures can get huge.The new
SERIALIZABLE_PROPERTIES
looked really neat, but I was surprised to see it's only used by the diff printer, and not the core eq logic itself.I admittedly do not have a bug reproducing exactly why that's problematic, but it does just seem unintuitive? Either from two perspectives:
SERIALIZABLE_PROPERTIES
to me infers it will be used "for serialization" which I would have expected to apply to both "the toEqual check" as well as "the toEqual diff", but alsoexpect-utils
eq
logic did not needSERIALIZABLE_PROPERTIES
to avoid blowing up on the getter from the original PR's use case (which FWIW we've definitely hit that bug too, that's why I care about this), then why didmatcher-utils
need a new custom property? Couldn't matcher-utils have just used expect-utils logic?I guess I don't have a true bug per se, so happy to close this out @georgekaran / @SimenB , but it just seems a little disjoint, i.e.:
toEqual
eq
but not the diff creationSERIALIZABLE_PROPERTIES
hooks into the diff creation, but nottoEqual
eq
I totally get evolving ubiquitously-used software over-time is basically impossible 😓 , but just seems like there is a lot of disjointness, and maybe
SERIALIZE_PROPERTIES
as a user-observable flag wasn't a necessary addition, ifmatcher-utils
-the-diff andexpect-utils
-the-equality reused more of their core functionality.Again, not a bug per se, just an observation that
SERIALIZABLE_PROPERTIES
doesn't work the way I expected, and I think for our ORM usage we'll either have to do both custom equality checker +SERIALIZEABLE_PROPERTIES
to gettoEqual
to do what we want. Which is fine too.Thanks!
Environment
The text was updated successfully, but these errors were encountered: