Support reference comparison
jakubzitny opened this issue · 8 comments
Right now, the .to.equal assertion after having chai-immutable in chai does "deep" comparison. It compares the keys and values of Maps, values of Lists and so on. This breaks the original behaviour of .to.equal in chai.
I understand that it is done on purpose, and test code often needs this more than reference comparison. However, when having chai-immutable in our specs, we can't compare references, only via expect(a === b).to.be.true which is not a good practice in "having readable assertions". There is even an eslint plugin for it.
It would be cool to:
- either change the
chai-immutable'sequaltoeqlordeep.equaland keepequalfor comparing references (this will break backward compatibility here, but will unify the behaviour with the rest ofchai) - or add a new method for comparing references, e.g.
.to.referenceEqual
Would you be open to a PR for any of these? Or is there any other problem or issue regarding this?
Hey @jakubzitny! Thanks for your feedback 🙏
I am totally open to a PR (I promise I will review it faster than I replied to your issue 😅). I prefer the option that maintains parity with the core of chai as this is pretty much what this plugin tries to achieve.
Wanna help? :)
Actually, thinking more about this, I think .to.equal should stay the way it is because that's more in line with what Immutable.js is.
I think comparing references is an edge case in the world of Immutable.js (may I ask you for a use case?) so it would be unfortunate to tie it to the default meaning of .to.equal I think.
At the end of the day, assuming a and b are Immutable.js object, it shouldn't matter if expect(a).to.equal(b) ensures the references are the same, it's all value-based. Maybe not being able to assert reference equality is a sign that something might be fishy in the specs, but if there needs to be an escape hatch, then an explicit .to. referenceEqual is probably more appropriate. I'd be curious about use case before having a definitive opinion however.
Our use case is covering shallow compare compatibility of our state. We have a store which returns a Map for instance and we want to ensure that it returns the same map instance on multiple invocations unless there were changes between getter calls. This allows us to optimize React component rerendering as the default comparison logic relies on reference equality.
Thanks for the answers. Well, the use case is simple — I wanna be sure that a method (from flux store, a custom collection, or any class that stores Immutable data internally and returns them filtered/modified/mapped) returns always the same "modified" collection and does not re-create it on call every time (unless the data has changed).
Or as @jankuca mentioned above.
I think changing the .to.equal behaviour would be more "correct", but in order to cover most cases with Immutable.js and also to keep the backward compatibility, I would really be better to just implement to.referenceEqual.
Gotcha, thanks for the clarification!
Agreed, let's go with to.referenceEqual, at least while in v2, and if it needs changing in the future, it could be done in v3. But in the meantime and if possible, I'd rather keep .to.equal as a value equality check since it's more in the spirit of Immutable.js.
Thank you both for your help, looking forward to a PR! :)
Hey @jakubzitny and @jankuca, quick follow-up: do you still intend to open a PR for this? I'd be happy to include it in the next release :)
Sure -> #218 😉