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

Alternate deep-comparison plugins should be easy to develop #1327

Closed
gibson042 opened this issue Oct 28, 2018 · 5 comments
Closed

Alternate deep-comparison plugins should be easy to develop #1327

gibson042 opened this issue Oct 28, 2018 · 5 comments
Labels
Category: API Status: Declined Type: Enhancement New idea or feature request. Type: Meta Seek input from maintainers and contributors.

Comments

@gibson042
Copy link
Member

gibson042 commented Oct 28, 2018

Deep comparison is a bundle of sharp edges. We make opinionated choices along certain dimensions for which individual use cases might have reasonable opposite preferences, and don't even document them except with references in comments to issues that successfully argued for new behavior. Some examples:

As long as deep comparison is bundled with the core project, there will continue to be bespoke requests like #1325. I'd much rather see it pulled out and given a proper treatment as an addon via immediate deprecation followed by removal in the next major release. Perhaps a well-documented dead-simple remnant that serves the common use case of SameValue equality across the values of all (and only) enumerable properties of plain objects and arrays could be left behind.

@rwjblue
Copy link
Contributor

rwjblue commented Oct 28, 2018

Hmm, IMHO it would be quite odd to have a testing framework without deep comparison capabilities. It’s a pretty common assertion and widely supported by other JS testing frameworks.

Relax comparison in certain cases ...snip... or for troublesome getters

I’d love to discuss the specific merits of #1326 with you over in the PR’s itself. 🙂

@trentmwillis
Copy link
Member

I agree with @rwjblue that it would be odd to not have this out of the box. That said, I also agree with @gibson042 that greater flexibility would be nice.

Given those two aspects, I'd be in favor or making it possible to "plug in" new comparison algorithms. We could even extract the current implementation into a "plug in" that is included by default so that it can have it's own set of documentation and even be consumed by other projects if desired.

@Krinkle Krinkle added Type: Meta Seek input from maintainers and contributors. Component: Assert labels Dec 14, 2018
@Krinkle Krinkle added Category: API Type: Enhancement New idea or feature request. and removed Component: Assert labels Oct 4, 2019
@Krinkle Krinkle changed the title Deprecate (not)deepEqual and (not)propEqual Alternate deep-comparison plugins should be easy to develop Oct 4, 2019
@Krinkle
Copy link
Member

Krinkle commented Oct 4, 2019

Which methods would we need to expose to make this easy?

Is it the recursive iteration logic, or is that simple enough that plugins would prefer to be in control of that themselves?

Is it the comparison logic itself, e.g. that we'd make a more abstract version of the one we have currently, and allow plugins to instantiate that with configuration options? That would make it a bit harder to maintain, but might make sense if the logic is complex and/or needs frequent changes.

Do we want to allow changing the algo used by the built-in methods? I'd prefer we avoid the same-named method taking on different meanings based on context. This way, it would remain easy to compose different test suites, and to keep code easy to understand without some special context that can change its behaviour. It also helps static analysis tools such as eslint-plugin-qunit if they can remain agnostic of this (which imagine would be fairly difficult to determine).

@Krinkle
Copy link
Member

Krinkle commented Nov 22, 2020

Declining for now. Making the behaviour of built-in assertion methods change based on a plug-in seems problematic I think long-term.

But any and all patches to make our "equiv" logic more re-usable are welcome to enable folks creating plugins to create their own "deep" equal assertioon methods.

@izelnakri
Copy link
Contributor

izelnakri commented Jul 6, 2022

@Krinkle I've just come across this library and it has orders of magnitude benchmark results over the alternatives: https://github.com/epoberezkin/fast-deep-equal#performance-benchmark

Can we build a benchmark suite for our own deepEqual/equiv and then perhaps test it against the top ones? I don't mind QUnit having a dependency for deepEqual logic, if its implementation decisions/differences are clearly documented and benchmarked for future possible improvements.

Krinkle added a commit that referenced this issue Sep 21, 2022
Krinkle added a commit that referenced this issue Sep 21, 2022
Krinkle added a commit that referenced this issue Sep 21, 2022
Krinkle added a commit that referenced this issue Sep 21, 2022
Krinkle added a commit that referenced this issue Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: API Status: Declined Type: Enhancement New idea or feature request. Type: Meta Seek input from maintainers and contributors.
Development

No branches or pull requests

5 participants