Conversation
|
awesome! :D remember: if > half a day :) |
b8c27be to
fd22c82
Compare
|
@diasdavid ready for review |
|
@dignifiedquire will defer my review until there are good benchmarks. Thank you for pushing this! :) |
|
Doing more work on this, so not ready anymore ;) |
fd22c82 to
2d0c802
Compare
|
@diasdavid tests are passing locally for node, so you can start reviewing :) |
8a2688e to
d6a7d98
Compare
|
Added new todo |
|
Should |
|
@nikuda yes it should :D |
|
Proto files buffer to string progress:
|
|
@nikuda awesome, thank you |
|
From IRC Glad that I mentioned that several times :) Have you added CI tests to guarantee that interop remains? What were the tests you performed to verify? |
This interop are the tests you wanted to create scripts for. I just start a go and js node manually and connect them manually. There are a lot of tests to verify that we interop with go on libp2p-crypto already if you look at the tests there. But for the full integration test we need a full go-ipfs node from |
|
@dignifiedquire reviewed a bunch of this PR. ipfs-block needs some changes that will cascade to to the remaining PRs that I haven't finalized reviewing (i.e removed the 'needs review label') |
|
Important to handle before moving forward with this PR - libp2p/js-libp2p-crypto#18 |
|
Now that the changes have happened we need to update https://github.com/ipfs/community/blob/master/js-project-guidelines.md accordingly |
|
Unblocked with libp2p/js-libp2p-crypto#29 |
|
All dependencies were merged and released, doing the last updates now 🎉 |
|
@diasdavid can I get some last review on this? |
|
@dignifiedquire all CI is failing, is that what you are expecting? |
package.json
Outdated
| "libp2p-ipfs": "libp2p-ipfs-browser", | ||
| "./src/core/default-repo.js": "./src/core/default-repo-browser.js", | ||
| "./src/core/components/init-assets.js": false, | ||
| "fs-pull-blob-store": false, |
There was a problem hiding this comment.
need it as it is used in the tests, and does no harm otherwise
| const hash = utils.isDaemonOn() | ||
| ? argv.key | ||
| : new Buffer(bs58.decode(argv.key)) | ||
| : mh.fromB58String(argv.key) |
There was a problem hiding this comment.
Wait, does this still apply? we have standardised the block API
There was a problem hiding this comment.
yes, tests will fail otherwise
| const mh = new Buffer(bs58.decode(argv.key)) | ||
|
|
||
| ipfs.block.del(mh, (err) => { | ||
| ipfs.block.del(mh.fromB58String(argv.key), (err) => { |
There was a problem hiding this comment.
block.del should support multihash as strings
There was a problem hiding this comment.
that's exactly what I'm doing here, decoding the multihash from a string
src/cli/commands/object/get.js
Outdated
| (cb) => utils.getIPFS(cb), | ||
| (ipfs, cb) => ipfs.object.get(argv.key, {enc: 'base58'}, cb), | ||
| (node, cb) => node.toJSON(cb) | ||
| ], (err, res) => { |
| (cb) => utils.getIPFS(cb), | ||
| (ipfs, cb) => ipfs.object.new(cb), | ||
| (node, cb) => node.toJSON(cb) | ||
| ], (err, node) => { |
src/http-api/resources/object.js
Outdated
| waterfall([ | ||
| (cb) => ipfs.object.get(key, cb), | ||
| (node, cb) => node.toJSON(cb) | ||
| ], (err, res) => { |
src/http-api/resources/object.js
Outdated
| waterfall([ | ||
| (cb) => ipfs.object.patch.setData(key, data, cb), | ||
| (node, cb) => node.toJSON(cb) | ||
| ], (err, res) => { |
src/http-api/resources/object.js
Outdated
| }, | ||
| (link, cb) => ipfs.object.patch.addLink(root, link, cb), | ||
| (node, cb) => node.toJSON(cb) | ||
| ], (err, res) => { |
src/http-api/resources/object.js
Outdated
| waterfall([ | ||
| (cb) => ipfs.object.patch.rmLink(root, link, cb), | ||
| (node, cb) => node.toJSON(cb) | ||
| ], (err, res) => { |
There was a problem hiding this comment.
Seems overly verbose, and I really dislike all caps naming. Not sure about a better name
There was a problem hiding this comment.
well, toJSON is also JSON all caps
There was a problem hiding this comment.
because apis in JavaScript are note very consistent when it comes to naming :/
| require('./test-files') | ||
| require('./test-generic') | ||
| require('./test-init') | ||
| require('./test-object') |
yes, waiting for the bitswap PR I pinged you about |
|
@dignifiedquire there you go ipfs/js-ipfs-bitswap#67 |
|
CI is finally green, browsers are not perfect atm, most are failing due to one or the other issue Firefox fails with this though this code runs fine in Chrome. @diasdavid if you are happy with the code please merge and ship at your earliest convenience, and I will investigate and fix various browser related issues next week. |
|
🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 |
* chore(package): update aegir to version 19.0.3 Closes ipfs#480 * fix: appease linter License: MIT Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
These are the changes that needed to bubble up from deep down in libp2p-crypto and
multihashing.This reduces the size of the browser build down to
2.4Mand1.2Munminified and minified respectively.Associated PRs that need to be merged and shipped before this can get merged are
TODOs
multihashing-asyncinstead ofmultihashing