Migrate to a pure Data Model API

[mikeal]

This would be a breaking change.

I’d like to use the new IPLD Schema for dag-pb for this codec.

This would be a major API break, but we probably need to do it because it would:

• make it much easier to use the codec since you’d be able to interact with regular JS objects rather Node instances
• align with the other implementations of dag-pb
• allow for selector engine support once the JS selector engine lands

However, this would be a pretty big break for js-ipfs. We’d want to time this along with a larger migration to the Block API.

/cc @achingbrain @vmx @alanshaw @carsonfarmer

[carsonfarmer]

Aside from the breaking changes to IPFS, I would say "yes please" to this proposal! Already most consumers of this module need to handle the 'Node' special-case to do the right thing, so it would be really nice to shed some of that cognitive overhead. We're already using the Block API in our internal (Textile) work, so less of a 'breaking' issue and more of a 'fixing' thing for us TBH.

[Gozala]

@mikeal can you clarify what's backwards incompatible about this change ? Talking to @rvagg I'm under impression that that this can be in fact backwards compatible change.

As I understand it it's merely treating anything shaped as

{
  Data?: Uint8Array,
  Links?: Array<{ Hash: Uint8Array, Name?:string, Tsize?: number }>
}

If that is indeed the case, most of the utils already work under than assumption. So it would be just matter of making corresponding getters an actual fields. That way DAGNode can still be kept around for backwards compatibility but can effectively be ignored.

If that is indeed this is all about, I'm happy to take it as that would unblock my work described in ipld/js-ipld#283

[Gozala]

As a side note I've just noticed that this implementation does seem to expect Buffer and fails on Uint8Array (because it's assumes it's a string). I would like to address that as well because once Buffers move across threads they turn into Uint8Arrays and I'd rather make dag-pb capable of handling Uint8Array`s than doing conversations the thread messaging layer.

[rvagg]

+1, all of our newest code is trying to be Buffer free where possible. js-multiformats doesn't use it all.

[rvagg]

^ I'd be fine with both of those things if you want to take it on and maintain backward compatibility. I'm a tiny bit concerned about the performance overhead of handling duck-typed DAGNode and DAGLink -like objects, but if you're careful that might not be a big deal. You'd probably go most of the way toward a new @ipld/dag-pb in doing this.

[mikeal]

You may run into problems trying to switch from Buffer to Uint8Array without a more substantial migration across js-ipfs. Even in js-multiformats we have a “legacy” function that wraps the new interfaces to match the old API and we convert all the Uint8Arrays to Buffers.

It’s a substantial breaking change to move to a new binary representation. IMO, it would be better to just wait until you can take the new js-multiformats work.

It’s also worth noting that if the binary type being returned from the top level API changes, that’s a breaking change in js-ipfs. It seems like it would be worth it to batch all of this together in a more substantial update.

[achingbrain]

I'm +1 on switching away from Buffer to Uint8Arrays.

We're coming to the end of an endeavour to replace the global node Buffer polyfill with the buffer module across our stack as webpack is going to stop including the polyfill in a future release, so we've got a rough handle on how big a task it would be.

Big, is the answer, there are a lot of modules to change: ipfs/js-ipfs#2924

[Gozala]

I have made changes to this library that

1. replaces getters / setters with non-writable properties so that that DAGNode / DAGLink complies with pure data model API but just happens to have a fancier prototype chain. See Backwards compatible pure data model API #184
2. changes util.serialize such that nodes data is casted to Buffer if it happens to be ArrayBuffer or ArrayBufferView. No copying just Buffer view over same underlying ArrayBuffer.

I also went ahead and wrote a patch for js-ipfs ipfs/js-ipfs#3070 to verify that these changes do not introduce regressions.

Finally I have incorporated these changes into my work on ipfs-message-port-* libraries and I'm glad to report that many tests are starting to pass (need to verify but I think some encountering different problems) without giving dag-pb any special treatment and despite the fact that nodes passed over message channel get their Buffer's swapped for Uint8Arrays's.

This give me some confidence in claiming that:

1. Proposed changes are backwards compatible.
2. These changes do address obstacles I was blocked by Sharing IPFS node across browsing contexts (tabs) on same origin ipfs/js-ipfs#3022 (comment)

There for I would like to make case for landing #184. Once it makes it's way to NPM I'll update ipfs/js-ipfs#3070 to swap dag-pb dependencies to point to released version instead of my forks.

[Gozala]

Fixed by #184

[rvagg]

I accidentally yak shaved my way to a new dag-pb implementation @ https://github.com/ipld/js-dag-pb, for use with the new multiformats & @ipld/block. It removes most of the custom object stuff from here and minimises the feature set so you're basically interacting with plain objects.