-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve toJSON of the CID #228
Comments
I would personally suggest to update toJSON as follows so it follows DAG-JSON Link representation toJSON () {
return {
'/': toString()
}
} |
I'm into returning the dag-json representation. 👍 Also @warpfork has been thinking about JSON IPLD more lately, not just with CIDs but also representing bytes. |
You mean differently from https://ipld.io/specs/codecs/dag-json/spec/#bytes ? |
@rvagg @achingbrain if you're onboard with this I can work on a PR |
I think this'd be a breaking change? I'm not sure if there are existing users of this functionality so it'd be good to look into that first before adding it in. |
It would be a breaking change, however I'm not sure where to look for the potential users of this API. I also suspect that toJSON got broken unintentionally somewhere down the line as I'm pretty user it did not used to contain Uint8Array serialized as an object. |
We could also just do this for |
I'm personally not scared of breaking changes when it's clear what has broken. Especially since this isn't a major API change. Just wondering if there are big projects we should loop in first. I think DagHaus is doing a lot with JS-IPLD right now so you'd be most likely to see this. |
At the moment
toJSON
returns following fieldshttps://github.com/multiformats/js-multiformats/blob/master/src/cid.js#L202-L208
Last one is Uint8Array, so when you run
JSON.stringify
over object containing CID it gets mangled into unreadable mess because bytes are turned into object with each byte in own offset property.Can we please either do something more useful in
toJSON
or drop that method entirely so that replacer function passed toJSON.stringify
can handle the substitution, which currently does not happen becausetoJSON()
is runs before replacer.The text was updated successfully, but these errors were encountered: