Skip to content
This repository has been archived by the owner on Sep 3, 2021. It is now read-only.

fix: allow CIDs to be compared through deep equality #132

Merged
merged 1 commit into from
Dec 11, 2020

Conversation

achingbrain
Copy link
Member

@achingbrain achingbrain commented Dec 11, 2020

The changes in #131 broke the tests of IPFS and probably quite a few other modules.

This sort of thing used to work, now does not:

expect(ipfs.bitswap.unwant.calledWith(new CID(cidStr), defaultOptions)).to.be.true()

The reason it breaks is because internally calledWith does a deepEqual on the args which compares (among other things) the properties of the passed objects.

We used to use Object.defineProperty to create cached versions of expensive to calculate fields which makes fields non-enumerable by default so they are skipped during the deepEqual check.

Now we just set the fields on the object which means instances have different fields depending on which constructor branch was hit or worse, if the instances properties have been accessed.

The changes in #131 broke the tests of IPFS and probably quite a few
other modules.

This sort of thing used to work, now does not:

```js
expect(ipfs.bitswap.unwant.calledWith(new CID(cidStr), defaultOptions)).to.be.true()
```

The reason it breaks is because internally `calledWith` does a `deepEqual`
on the args which compares (among other things) the properties of the passed
objects.

We used to use `Object.defineProperty` to create cached versions of
expensive to calculate fields which makes fields non-enumerable by
default so they are skipped during the `deepEqual` check.

Now we just set the fields on the object which means instances have
different fields depending on which constructor branch was hit or worse,
if the instances properties have been accessed.
@achingbrain achingbrain force-pushed the fix/make-comparison-work-again branch from 646c766 to 1766ec0 Compare December 11, 2020 14:11
Copy link
Member

@vmx vmx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick fix!

@vmx vmx merged commit 127745e into master Dec 11, 2020
@vmx vmx deleted the fix/make-comparison-work-again branch December 11, 2020 14:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants