js-multiformats icon indicating copy to clipboard operation
js-multiformats copied to clipboard

Proposal: Make `Link` and `Block` interfaces compatible

Open Gozala opened this issue 2 years ago • 5 comments

As things stand today it is impossible to satisfy both Link and Block interfaces with a same instance, because both they collide on bytes field:

https://github.com/multiformats/js-multiformats/blob/15c52c8a5384e1bc84cccfa34ed6c7a78eb23978/src/block/interface.ts#L47-L55

https://github.com/multiformats/js-multiformats/blob/15c52c8a5384e1bc84cccfa34ed6c7a78eb23978/src/link/interface.ts#L21-L42

This is unfortunate because we're finding ourselves in a position where we would like to satisfy both interfaces along with following DAG interface:

interface DAG {
   links(): IterableIterator<Link|Block>
}

More broadly speaking we would like an ability to give a link with it's data attached (when we have it).

Gozala avatar Dec 05 '22 21:12 Gozala

I think possible solutions here are:

  1. Rename bytes in either or both to have something less generic & non colliding.
  2. Drop bytes from Link and come up with something else for link['/'] === link.bytes
    • Perhaps link['/'] instanceof Uin8Array is good enough & is also backwards compatible.
  3. Drop bytes from Block in favor toBytes(): Uint8Array.
    • Perhaps we can drop cid in favor of link(): Link while we're at it.

Gozala avatar Dec 05 '22 21:12 Gozala

@Gozala I agree that this is worth fixing, but I can't bring myself to prefer a solution. Both of these things have .bytes and both feel like they should be named that way. Since Block class doesn't get much use currently, at least in comparison to CID (and therefore Link), any breakage there is going to be much lower impact. So a toBytes() or getBytes() might make sense. I'll leave it to you to be creative but I'm happy to move forward with this because what you're wanting to do with your DAG iterator makes a lot of sense and you already flagged that during the Link work and I thought it was a great idea.

rvagg avatar Dec 06 '22 23:12 rvagg

My current plan is to idea that motivated and make more informed and proposal based on that experience.

I thought starting this thread in parallel was a good idea to gather input from collaborators. I've been also trying to get some ideas from Node interface but so far I have failed to borrow much. But here is some rough plan:

  1. I need separate Link, Block and Node (which I previously called DAG) interfaces.
  2. In most codecs I use I already have write function that returns Block as opposed to just bytes, I would like to upgrade it to return Link & Block & Node instead.
  3. Bunch of our code works with higher level abstractions however, I call them views because they provide domain specific view of the Block.
    • We often find ourselves needing to go from View layer to block & links layer, I hope that our views could extend Link, Block and Node interfaces to address this.
  4. We don't depend on any outside code that expects Blocks so we can prototype Link & Block compat simply by changing our Block definition and learn from that experience.
  5. We could also evaluate if this hybrid constructs recognized as links by codecs are good idea or a bad one.

Gozala avatar Dec 07 '22 07:12 Gozala

Personally I'm leaning towards option 2, which I'd re-frame as:

  1. Deprecate bytes field in CID class
  2. Drop bytes field in Link interface (it's new enough to not cause much problem I think)
  3. Change CID-ness check to link['/'] instanceof Uin8Array
    • I highly doubt false positives, but if it turns out to be problem we could offer some opt-out field

Note that if bytes field is simply deprecated and not used by codecs Link & Block could put block bytes in bytes and have no issues.

Gozala avatar Dec 07 '22 07:12 Gozala

Just looking through my ipld js folders and I'm seeing cid.bytes show up pretty frequently—although of course they're mainly low-level things which don't necessarily reflect how end-users are going to be touching these things. But all the codecs use it, js-car uses it, anything that needs to do something encody with a CID. So it's going to be fairly disruptive to migrate away from it.

Do you have a proposal for a new interface for CID that gives access to the bytes? Am I going to have to cid['/'] to get access to this? That's not a great option, it makes the code more opaque. Are we going to have to go back to cid.buffer?

rvagg avatar Jan 02 '23 06:01 rvagg