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

Abstraction of CID to provide several returning types #209

Closed
wants to merge 2 commits into from

Conversation

clostao
Copy link
Member

@clostao clostao commented Dec 19, 2024

@jfrank-summit @marc-aurele-besner What do you think about this solution for the issue of easing the conversion into blake3hash, CID string or any other derived form of CID. For asynchronous functions should work fine.

We had commented about using an optional verbose argument but I don't like quite much varying the returning type depending on the input.

@clostao clostao changed the base branch from main to improve-observable-type December 19, 2024 18:52
@clostao clostao marked this pull request as draft December 19, 2024 18:57
@marc-aurele-besner
Copy link
Member

I'm not personally a fan of having to deconstruct multiple level of the response simply to get the cid, and I think returning 2 data identifiers by default may cause more confusion than problems it'll solve.

So I strongly believe cid should remain in the first level of return parameters

If calculating the blake3 hash add any noticeable delay (as I'm assuming it does, since it moved to async with the dynamic import of the package), it should be an optional return parameter, or simply making sure the dag-data package export a simple function to convert a cid to blake3 and add a example in the docs

@clostao
Copy link
Member Author

clostao commented Dec 19, 2024

I don't understand what you mean with the several levels of deconstruction? The cid field would be an object.

Additionally the delay on the import statement could be none if moved to the top level, however the async/await will still be needed.

@marc-aurele-besner
Copy link
Member

I don't understand what you mean with the several levels of deconstruction? The cid field would be an object.

Additionally the delay on the import statement could be none if moved to the top level, however the async/await will still be needed.

The observable is an object, now if the cid parameter is also an object, you need to deconstruct it to get the cid string.

I believe that returning two identifiers (cid string and black3 hash) by default, will simply create confusion.

If we assume that 80%+ users will just need the CID, then we should design for this, so that mean, making it possible to get the blake3 hash from a cid, but not making a default behaviour.

@@ -79,7 +80,7 @@ type ObjectUploadStatus<Type extends 'file' | 'folder'> =
completed: true
type: Type
progress: number
cid: string
cid: AutoCID
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cid should remain a string, if you want to add an optional hash parameter that get return only if some optional input parameter is present, I'm fine with that.

But I don't want cid to become an object or to return the hash by default, I think this will be a step backward for the devEx,

@clostao
Copy link
Member Author

clostao commented Dec 19, 2024

Actually, it wouldn't be deconstruction it just need to call asString method of AutoCID object similarly to what multihash's CID object does (without the need to pass the codec information).

I don't see a huge complexity in this object and the other two options:

  • Having an optional parameter: I feel the complexity of this one is considerably higher than returning an object. If you want to transform the output of a function (cid string) you'd expect to apply some transformation over it not over the input of the function.

  • Having a function that converts: For me this would be the best solution but due to cjs/esm issues the usage of this method is gonna have terrible devEx (for CJS devs). Moreover, getting the user to know this method isn't trivial.

@clostao clostao closed this Dec 19, 2024
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 this pull request may close these issues.

2 participants