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

feat: add createNode() and createLink() factories #20

Merged
merged 1 commit into from
Jun 24, 2021

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Jun 22, 2021

To match DAGNode and DAGLink constructors of old

See the README notes for examples of how it would be used, it's really just a shorthand for making the objects but does so safely and reliably.

I also considered moving prepare and validate and these new functions on to a @ipld/dag-pb/util export rather than the main export, but 🤷.

to match DAGNode and DAGLink constructors of old
@rvagg
Copy link
Member Author

rvagg commented Jun 24, 2021

I think I'm pretty happy with this, it almost does away with the need for prepare as an export and simplifies code, reducing magic object properties. Will Update ipfs/js-ipfs-unixfs#116 with a commit using this to demonstrate.

@rvagg rvagg merged commit 5084446 into master Jun 24, 2021
@rvagg rvagg deleted the rvagg/create-factories branch June 24, 2021 04:14
@achingbrain
Copy link
Member

achingbrain commented Jun 24, 2021

🤷 I kind of liked that I was passing plain old js objects into dagPb.encode and not having to use classes or special functions that return something that may be non-obvious. It seemed like a simple and intuitive way of doing things to me.

The only improvement I would make is maybe not having it require a Links property.

I think prepare and validate could be removed, they haven't really been necessary for the multiformats migration work I've been doing.

@rvagg
Copy link
Member Author

rvagg commented Jun 24, 2021

k I don't mind either way, as long as we get to strict forms in the end that's all I care about. If you want to back out the change to ipfs/js-ipfs-unixfs#116 I'm fine with that and may back out this change if it's not being used there.

* @returns {PBNode}
*/
export function createNode (data, links = []) {
return prepare({ Data: data, Links: links })
Copy link
Contributor

@Gozala Gozala Jun 29, 2021

Choose a reason for hiding this comment

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

I literary meant this:

Suggested change
return prepare({ Data: data, Links: links })
return { Data: data, Links: links }

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason for the prepare() is simply to ensure the data types and shape are correct, do some basic coercion where possible and to throw an error if this won't pass encode(). It's essentially doing { Data: data, Links: links} but with checking too so I don't have to duplicate that logic. So the helpers are not just giving you correctly shaped objects but are checking the values you pass it too.

* @returns {PBLink}
*/
export function createLink (name, size, cid) {
return asLink({ Hash: cid, Name: name, Tsize: size })
Copy link
Contributor

@Gozala Gozala Jun 29, 2021

Choose a reason for hiding this comment

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

And this

Suggested change
return asLink({ Hash: cid, Name: name, Tsize: size })
return { Hash: cid, Name: name, Tsize: size }

* @param {CID} cid
* @returns {PBLink}
*/
export function createLink (name, size, cid) {
Copy link
Contributor

@Gozala Gozala Jun 29, 2021

Choose a reason for hiding this comment

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

Suggested change
export function createLink (name, size, cid) {
export function createLink ({ name, size, cid }) {

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea here was to provide something that had the same function signature as the old PBLink and PBNode constructors in ipld-dag-pb.

* @param {PBLink[]} [links=[]]
* @returns {PBNode}
*/
export function createNode (data, links = []) {
Copy link
Contributor

@Gozala Gozala Jun 30, 2021

Choose a reason for hiding this comment

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

Suggested change
export function createNode (data, links = []) {
export function createNode ({ data, links = [] }) {

@rvagg
Copy link
Member Author

rvagg commented Jul 6, 2021

OK, so neither of you are happy with this, I'm not overjoyed myself. Should I just remove them entirely? @Gozala @achingbrain?

@achingbrain
Copy link
Member

The only extra thing I want out of this module is to not require a Links property:

dagPb.encode({
  Data: Uint8Array.from([0, 1, 2])
})

I would even skip validation on encode entirely as it then guards on all the properties anyway - if the user cares they can call validate themselves. I think this is consistent with the yolo approach IPLD takes to validation in other places (ref: all the is-it-a-string-or-is-it-a-byte-array discussions).

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.

3 participants