Skip to content
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

Closed
Gozala opened this issue Dec 12, 2022 · 8 comments · Fixed by #230
Closed

Improve toJSON of the CID #228

Gozala opened this issue Dec 12, 2022 · 8 comments · Fixed by #230

Comments

@Gozala
Copy link
Contributor

Gozala commented Dec 12, 2022

At the moment toJSON returns following fields

https://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 to JSON.stringify can handle the substitution, which currently does not happen because toJSON() is runs before replacer.

@Gozala
Copy link
Contributor Author

Gozala commented Dec 12, 2022

I would personally suggest to update toJSON as follows so it follows DAG-JSON Link representation

 toJSON () {
    return {
      '/': toString()
    }
}

@RangerMauve
Copy link
Collaborator

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.

@Gozala
Copy link
Contributor Author

Gozala commented Dec 12, 2022

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 ?

@Gozala
Copy link
Contributor Author

Gozala commented Dec 12, 2022

@rvagg @achingbrain if you're onboard with this I can work on a PR

@RangerMauve
Copy link
Collaborator

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.

@Gozala
Copy link
Contributor Author

Gozala commented Dec 12, 2022

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.

@Gozala
Copy link
Contributor Author

Gozala commented Dec 12, 2022

We could also just do this for Link and leave CID alone, but then again I think it is a problem that JSON.stringify produces a mess with CIDs and that problem would persist if we choose to leave CID alone.

@RangerMauve
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants