Skip to content
This repository has been archived by the owner on Aug 11, 2021. It is now read-only.

Converting string input data to Buffer. #11

Merged
merged 2 commits into from
Mar 9, 2017

Conversation

JeffDownie
Copy link
Contributor

Basically, I ran into this because I mis-read the README and started putting in string data into DAGNode.create! If you do this, the current behaviour allows a number of odd/confusing scenarios, and it's not clear that you've done something wrong.

Currently, if a string is put in as the data, it will be accepted, and will work when put or gotten from IPFS, but it will break if you try and clone the DAGNode due to the line dagNode.data.copy(data) (js strings don't have a copy method).

Passing in string data seemed like a reasonable thing to do, but I felt converting the input data string to a buffer would be better than allowing the internal DAGNode data to be a string - I felt having two internal data representations would perhaps cause other unknown problems, however, I am open to reverse this!

Also fixed a small typo in the README, because why not :)

Copy link
Member

@daviddias daviddias left a comment

Choose a reason for hiding this comment

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

Let's do even more strict and force it to be always Buffers, which is what the protobuf expects anyway. This has bitten us in the past.

//cc @haadcode

Also added vim .swp files to .gitignore
@JeffDownie
Copy link
Contributor Author

I've added a check to ensure that data is a Buffer (after which argument is the callback has been worked out) - was that what you meant?

@daviddias
Copy link
Member

@JeffDownie exactly :)

@daviddias
Copy link
Member

added a notification for those following API changes here: ipfs-inactive/interface-js-ipfs-core#55 (comment)

Will give it a day before merging, if no one manifests anything against, of course :)

@daviddias
Copy link
Member

Seems that no one raised any problem, going ahead and merging this. (sorry for having took so long!)

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

Successfully merging this pull request may close these issues.

2 participants