feat: asCID property self reference

[mikeal]

This came up in a conversation I had with @Gozala

This feature is nice for a few reasons.

1. it’s a relatively unique property you can check that doesn’t rely on symbols.
2. this makes the toJSON() representation serializable through a Worker but, since it includes a circular reference, can still be understood as a CID instead of just a regular object.
3. this breaks CID serialization and causes an exception when you try to naively encode them as objects.

However, if we take this change it will require some updates to our codecs. In both dag-cbor and in dag-json we do an isCircular check before encoding an object and that runs through a relatively naive third party library that will now throw. But it’s already in my mental backlog to get rid of is-circular so it’s probably fine.

[Gozala]

Looks good to me! I also have sketched what we discussed in the call here
multiformats/js-cid#111 (comment)

[mikeal]

Is it confusing to use the same property name for the instance and static class method?

[Gozala]

Is it confusing to use the same property name for the instance and static class method?

I don't think so, as property is mostly for static method to recognize CID. That being said I really could not care less about property or static method name. Mostly went with asCID as it's one char different from isCID :)

[rvagg]

I guess so ... it's pretty user-hostile though to be doing this in toJSON(), what does that look like if someone's using JSON.stringify() on a parent object for debugging?

[mikeal]

I guess so ... it's pretty user-hostile though to be doing this in toJSON(), what does that look like if someone's using JSON.stringify() on a parent object for debugging?

That’s a good point.

Maybe we leave it out of toJSON() and just leave it enumerable. Most encoders probably enumerate the properties rather than using toJSON().

[jacobheun]

Should this be merged or do we need resolution on toJSON? It's been approved and sitting for a while.

[jacobheun]

Also, is the plan to replace the cids module everywhere? If so, is there a rollout plan for that?

[mikeal]

Should this be merged or do we need resolution on toJSON? It's been approved and sitting for a while.

I’ll be returning to it eventually, other priorities have been getting in the way. I started this PR to fully consider the interface and I don’t feel like I’ve really had the time to do that yet.

Also, is the plan to replace the cids module everywhere? If so, is there a rollout plan for that?

Yes, some day, but not today :)

For js-ipfs this shouldn’t get adopted any time too soon, and should be part of a much larger change migrating to all the new Block and multiformats interfaces. But it’s premature to consider that given that we haven’t even updated our own stack to these changes.

I’ll join the IPFS implementations call on Monday to discuss these changes in more detail.

[vmx]

I'm strongly against breaking toJSON().

[mikeal]

I'm strongly against breaking toJSON().

Ya, we won’t be, if this lands it’ll land without the toJSON() break.

[vmx]

Please give the commit message a better header and a bit more context.