This repository has been archived by the owner on Aug 11, 2021. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Major API changes #185
Major API changes #185
Changes from all commits
5139f53
8165bf9
221fab0
1a2c0ad
2365ae3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not return the full block instances? It doesn’t really cost us anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to expose the serialized form of the nodes like that? I don't think it's needed from the IPFS point of view but are there use cases where this is desirable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's actually a good point. When using IPLD you shouldn't really care about how it is serialized. I'm always in favour having a minimal API :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly - it doesn't feel like an application-level concern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would agree if the API only returned the final value it had resolved, but it doesn’t, it returns all the CID’s it passed through to resolve the path. It’s already exposing the serialization just by giving you the CID’s of the intermediate nodes and in that case, why not just give me the blocks if you already have them? It costs nothing on the consumer side to ignore the content and just use the CID and it saves a BlockService lookup if you do need the data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me it's about abstractions. In js-ipld you only care about the the IPLD Data types. A CID is as low as you go, you don't care about how it is serialized, but only about that it got serialized.
IPLD Formats are the low level building block, which is about serialized data. You should use that level if you have needs to operate on that level.
I think not exposing the serialized data would make IPLD easier to use as it is one less concept to learn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mikeal This discussion is the last outstanding one. Would you be OK with moving forward with the current state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya, it’s fine, go ahead and merge.