-
Notifications
You must be signed in to change notification settings - Fork 299
use object tests from interface-ipfs-core #267
Conversation
4d3cc27
to
11a75d1
Compare
return callback(new Error('Stored object was different from constructed object')) | ||
} | ||
|
||
callback(null, node) |
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.
In order to support the variety of ways ipfs.object.put
can be called, I had to break the elegant promise injection that was there before. @dignifiedquire wanna share some wisdom on how we can add promises again, in the most elegant way possible?
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 probably best to just wrap the function using sth like this: https://github.com/manuel-di-iorio/promisify-es6
It took me more time to do object.put than I thought, spent too much time looking at the promises thing and I believe that we this new Interface document and tests, I caught a bunch of scenarios where the hash returned and the possible hash created client side would not match, not this last part is all good :) |
if (!options.enc) { | ||
tmpObj = { Data: obj.toString(), Links: [] } | ||
} else { | ||
tmpObj = JSON.parse(obj.toString()) |
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.
this should only happen if enc === 'json'
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.
wait no, we actually don't want to parse at all in here, that's the job of ipfs itself isn't it?
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.
confused
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.
This is for the use case (that people have been doing forever) that goes like:
const obj = {
Data: new Buffer('Some data').toString(),
Links: []
}
const buf = new Buffer(JSON.stringify(obj))
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.
But why do you need to parse that, you can just pass that to go-ipfs with the encoding
646f7c6
to
50bc13a
Compare
Tests complete. @dignifiedquire @haadcode mind CR'ing this one? This will be the API breaking change that will enable you to jump from js-ipfs-api to js-ipfs with a simple line, @haadcode :) |
54f90cd
to
7852050
Compare
|
||
send('object/stat', multihash, null, null, callback) | ||
}), | ||
new: promisify((callback) => { |
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.
what about layouts?
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 tried all the other layouts through the CLI and only unixfs-dir
works. It seems one of those deprecated features that no one ever used to even notice. I updated the spec accordingly.
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.
Hmm, I made it fully work in js-ipfs :/
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.
Would you like to propose that we shim that feature in, although go-ipfs not supporting? All is possible with code 🎤 :D
7852050
to
88a31d0
Compare
greeen :D |
88a31d0
to
2000d22
Compare
ok, merging this one :D |
WIP