fix: remove use of Object.defineProperties in CID class

[achingbrain]

Object.defineProperties is a performance bottleneck in applications that create lots and lots of CIDs (e.g. IPFS) so this PR removes it.

The asCID property is changed to be a private class field which requires increasing the minimum supported EcmaScript version but I don't know if that's a big deal or not. It does seem to make the property non-enumerable though.

The CID class now implements a new Link interface that has public read-only byteOffset and byteLength properties so these become regular properties. Similarly code, version, multihash and bytes become writable/configurable but they are also marked with @readonly so maybe that's enough?

Fixes #200

[achingbrain]

Targets the upcoming v10.0.0 branch and not master so needs #199 merging first.

[achingbrain]

Before:

After:

Much faster!

[rvagg]

I'm fine with this I think, it's a breaking change but we could bundle it into 10.0.0 safely I think. The byteOffset and byteLength properties are going to be the major actual break here as they'll now be showing up when enumerating properties. Hopefully there's not a common case for doing that though, I can't think of one.

[achingbrain]

If it's a problem, you can do the whole prefix-with-# + getter trick that's being done with asCID to makebyteOffset and byteLength non-enumerable, but they're now part of the Link interface that CID implements so I guess they should be enumerable?

[achingbrain]

I have to admit, I'm a little confused by the Link interface, I thought CID was going to be an interface?

[rvagg]

Discussion on Link in #161 (comment) - the biggest selling point is that when consuming a CID as an interface while also trying to use the concrete form, the naming is a mess. You end up with something like CIDIface & CID or CID & CIDImpl. It's fine when you don't need to use CID and just want to describe interfaces that deal with it, but when the two come together it's not pretty.

Feel free to speak up if you disagree and want to make a case for this increasing complexity or confusion or?

[achingbrain]

I'll open a separate issue, it's unrelated to this PR

[achingbrain]

@rvagg #203

[rvagg]

@achingbrain are you currently shipping private class fields anywhere in distributed code in js-ipfs or js-libp2p? We don't do a compile step here now so this is going straight out to users and will be the base of almost everything in our stack. How many users are we going to give headaches to? Node 12 and Chrome 74, but what about all the edges of old Safari etc. that we want to push our stack to? .. i.e. are we ready for the hadache?

[rvagg]

I'm also remembering that we have uses of asCID in the wild: https://github.com/ipld/js-dag-cbor/blob/47168509e6571c729604ebc29013f314f618379d/index.js#L21

Probably not the worst thing to just remove that, but it has been handy as a very fast initial check that this thing might be a CID.

[rvagg]

oh, this is making it public again? so this is essentially a hack to make a hidden property? sneaky.

[Gozala]

I don't think this is going to survive the structural cloning which is what going to occur when transferring CIDs across realms/threads.

If it does not it would defeat the purpose

[achingbrain]

@Gozala are there any tests that guard against regressions around this? I can't see anything obvious.

[Gozala]

We should have, but we did not as far as I can tell. I have created one #206

[Gozala]

BTW I don't believe there is any problem with making asCID public other than it would break JSON.stringify(cid) which could be addressed by having toJSON method.

So maybe it's worth just making it a regular property and call it a day.

[rvagg]

yeah, ok, so let's just go with this.

• private + getter trick takes care of asCID although looking at the uses of it I reckon we could even just make it fully private, but let's defer that
• re ecmascript versioning - we're going to be inflicting esm on direct consumers of this library anyway so likely anyone trying to support older runtimes will be transpiling anyway

[achingbrain]

are you currently shipping private class fields anywhere in distributed code in js-ipfs or js-libp2p

I thought we did but - the syntax is certainly used but it's all typescript and looking into the published code tsc has converted it to use WeakMaps.

[achingbrain]

I'm also remembering that we have uses of asCID in the wild: ipld/js-dag-cbor@4716850/index.js#L21

I might be missing something but is this not the kind of place you'd just pass it to CID.asCID and null-guard on the output instead of examining cid.asCID?

[Gozala]

Can check some assumptions here ?

1. Is asCID getter route faster than Object.defineProperties ? I realize it's only triggered on access, yet if we always check for cid.asCID is there going to be a win here ?
2. Why does asCID needs to be non-enumerable / non-writable ? If I recall correctly I have only did that because other properties were doing it, but then again if we're ok with making other properties regular can't we do the same for this one ?

IMO As long as we keep toJSON around we should feel free to use regular property here. If users mess with it, they should expect problems and we can add JSDoc comments to deter that.

[rvagg]

I might be missing something but is this not the kind of place you'd just pass it to CID.asCID

yeah, it's certainly not necessary and really just exists as a super-shortcircuit since that function is operating on the full object graph so will be hitting lots of objects that may or may not be a CID, so we get to do a quick simple check and avoid additional logic in the adCID function .. which isn't that much, but it's more than that check.

Why does asCID needs to be non-enumerable / non-writable

I don't recall the original justification for hiding them, it may be have been to do with https://github.com/ipld/is-circular which I think we've now migrated off anyway. I'm inclined toward a Chesterton's Fence one this one given there may be dragons, but the question about the performance tradeoff is a good one - CID.asCID() does some heavy lifting now since we're using it all over the place and it's supposed to be fast. Is the getter for asCID going to slow that down? @achingbrain do you happen to have before and after profiling for that static function like you showed for the constructor?

[Gozala]

@rvagg my problem with getter isn’t a performance, but the fact that it will be lost when moving realms as illustrated by #206

[BigLep]

2022-10-04 IPLD triage note: @rvagg is going to sync with @achingbrain about making asCID as regular property.

[rvagg]

#206 was merged into here, demonstrating the realm problem.

I've simplified the code here and made asCID a regular property, no fanciness; let's introduce fancy later if we find a genuine use-case for it. See fb69544.

Merging it with those change in to #199, will pause for feedback before we release.