search

ipld / js-dag-cbor

dag-cbor codec for IPLD
Other
26 stars 17 forks source link

Unable to add custom tags #12

Closed carsonfarmer closed 3 years ago

carsonfarmer commented 3 years ago

It looks like the exported function configureDecoder should support adding custom tags, but currently the tags on options are ignored: https://github.com/ipld/js-dag-cbor/blob/master/index.js#L89.

I would be happy to submit a PR if this is functionality that would be useful generally?

rvagg commented 3 years ago

eeeeh, that jsdoc is wrong and the tags option should probably be removed entirely from that. I guess that's a remnant of internal usage where we want to set it internally but not expose it externally, js-dag-cbor should not support any additional tags so we shouldn't even expose the option.

Or do you have some novel usecase that might justify it?

carsonfarmer commented 3 years ago

I'm not sure I have all that novel of a use-case top be honest, I just wanted to be be able to serialize Sets as Arrays with a specific tag. See https://www.iana.org/assignments/cbor-tags/cbor-tags.xhtml.

rvagg commented 3 years ago

Since we restrict DAG-CBOR (0x71) to just the tag for CID then any additional tags would make invalid DAG-CBOR. We don't yet have a proper CBOR (0x51) codec where this might be acceptable (although that'd be up for discussion too - does 0x51 support all the tags in the spec? does it support all the tags in the registry??) so maybe it'd be worth publishing one, like we have for JSON (0x0200), which comes with js-multiformats.

There's a couple of other things that borc currently affords that shouldn't be allowed, and would be rejected by the Go implementations of DAG-CBOR (like undefined), and we're going to tighten them up. There's even a discussion that's looking like we might be rejecting Infinity, -Infinity and NaN too.

Perhaps this is the right place to export such a codec, so we might leave this issue open. It'd just have to be clearly distinct from DAG-CBOR.

carsonfarmer commented 3 years ago

Ah yes, gotcha. This all makes sense (the distinction between 0x71 and 0x51 especially). Thanks for the clarifications, and for keeping this one open for further visibility/discussion.

BigLep commented 3 years ago

This should be tagged with a "discussion" label. @jacobheun : can you please help tag it (I'm working to get proper permissions on the repo.)

lidel commented 3 years ago

Closing, this repo predates the spec, but the spec itself allows only for CID tag, so nothing to do here going forward.