-
Notifications
You must be signed in to change notification settings - Fork 780
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
Comments
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.
I’d love to discuss the specific merits of #1326 with you over in the PR’s itself. 🙂 |
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. |
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). |
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. |
@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 |
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:
deepEqual( Object.create(null), {} )
,deepEqual( Object.create(Object.create(null)), {} )
,deepEqual( new Uint8Array([1]), new Uint16Array([1]) )
)?writable
,configurable
,value
vs.get
)?constructor(){ this.method = function(){…}; }
or for troublesome getters or [I'm sure this is coming] Number vs. BigInt)?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.
The text was updated successfully, but these errors were encountered: