Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

Implementing the new interfaces #1086

Merged
merged 33 commits into from
Nov 20, 2017
Merged

Implementing the new interfaces #1086

merged 33 commits into from
Nov 20, 2017

Conversation

daviddias
Copy link
Member

@ghost ghost assigned daviddias Nov 17, 2017
@ghost ghost added the status/in-progress In progress label Nov 17, 2017
callback(null, toStream.source(files[files.length - 1].content))
})
p,
pull.concat(callback)
)
}),
Copy link
Member Author

Choose a reason for hiding this comment

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

@pgte the .cat for a large file (15Mb) is failing with:

image

Seems that some of the streams is giving up too early. Could you help me figuring this one out?

Copy link
Member Author

Choose a reason for hiding this comment

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

Think I got it, I can't use concat to concat buffers https://github.com/pull-stream/pull-stream/blob/master/sinks/concat.js

@daviddias
Copy link
Member Author

daviddias commented Nov 17, 2017

What is missing in this PR:

  • Fix big files tests
  • Finish migrating exchange files to pull-streams
  • Update remaining tests that use add, cat and get

@daviddias
Copy link
Member Author

daviddias commented Nov 17, 2017

Now the example is using pull-streams directly (though ipfs.files.add) but I'm being unsuccessful to add and fetch files (although tests are 100% passing). Something might have changed in the browser APIs that is messing with the data blobs.

@dignifiedquire @victorbjelkholm does this ring a bell?

@daviddias daviddias requested review from pgte and ya7ya November 17, 2017 14:07
pgte
pgte previously approved these changes Nov 17, 2017
Copy link
Contributor

@pgte pgte left a comment

Choose a reason for hiding this comment

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

LGTM, only optional minor improvements on the examples.

node.on('ready', () => {
console.log('IPFS node is ready')
})
node.on('ready', () => console.log('IPFS node is ready'))
Copy link
Contributor

Choose a reason for hiding this comment

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

What about using .once instead of `.on´?


node.start(() => {
console.log('Online status: ', node.isOnline() ? 'online' : 'offline')
node.on('ready', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use .once instead?

node = new IPFS({
repo: String(Math.random() + Date.now())
})
node = new IPFS({ repo: String(Math.random() + Date.now()) })

node.on('ready', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

once would be nicer, I think.

// EXAMPLE
// node = new self.Ipfs({ repo: 'ipfs-' + Math.random() })

node.on('start', () => node.id((err, id) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

what about .once?

const parallel = require('async/parallel')
const IPFSFactory = require('../utils/ipfs-factory-instance')

describe('verify that kad-dht is doing its thing', () => {
describe.skip('verify that kad-dht is doing its thing', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

This tests needs to be unskiped and passing before merging

@daviddias
Copy link
Member Author

daviddias commented Nov 17, 2017

Update, I'm down to:

HTTP npm run test:unit:node:http

screen shot 2017-11-17 at 16 27 39

CLI npm run test:unit:node:cli

screen shot 2017-11-17 at 16 36 59

The HTTP and CLI failures are related, fixing one will fix the other. Both of them throw the following error:

screen shot 2017-11-17 at 16 35 48

@dryajov
Copy link
Member

dryajov commented Nov 17, 2017

@diasdavid

Discovered with this PR's that http, gateway and CLI tests were being skipped in CI since the Circuit Relay PR, enabling them found a bunch of bugs, fixing them now.

Were those ever enabled? I can't trace them to being enabled either in package.json nor travis/circle.

@daviddias
Copy link
Member Author

@dryajov yes, they were enabled through the node.js file that was present in the test folder (like it is now)

console.log('Fetched file:', cid, file.content.length)
// calling createFileBlob makes the Chrome go "Oh Snap"
// const listItem = createFileBlob(file.content, cid)
// $filesList.insertBefore(listItem, $filesList.firstChild)
Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, narrowed down to the createFileBlob func. Apparently it always makes the Chrome go "oh Snap" and in Firefox it creates a wrong file (3x the size)

@victorbjelkholm thoughts on this one?

@daviddias
Copy link
Member Author

Good news everyone!

I was successfully able to upload a file with more than 750Mb (didn't test higher) and was able to fetch it through the gateway (slowly but it went through).

console.log('packed a directory')
cb()
})
}
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 final issue is here, for some reason, tar-stream blocks on the second dir that is added:

image

@daviddias
Copy link
Member Author

daviddias commented Nov 20, 2017

Fixed it 🌟

The issue was introduced when I adopted the getPullStream for the http-api (instead of the old with ReadableStreams) and although both implemented just fine, the end of a toStream.souce(pull-stream).pipe(tarPackStream) was telling the whole pack that the thing had finished already and so it would stop after the first file.

tl;dr;, Readable Streams are dangerous, mixing them with pull-streams doesn't make it better, gotta be extra cautious.

@daviddias daviddias merged commit 2c4b8b3 into master Nov 20, 2017
@daviddias daviddias deleted the new-streaming-interface branch November 20, 2017 11:07
@ghost ghost removed the status/in-progress In progress label Nov 20, 2017
MicrowaveDev pushed a commit to galtproject/js-ipfs that referenced this pull request May 22, 2020
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
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.

4 participants