-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Conversation
@@ -36,7 +36,7 @@ module.exports = Command.extend({ | |||
throw (err) | |||
} | |||
res.on('data', (data) => { | |||
data.stream.pipe(process.stdout) | |||
data.content.pipe(process.stdout) | |||
}) |
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 will be changed to make res the stream instead, once we have the interface-ipfs-core for cat merged too
//cc @nginnever
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.
Yup, changes to make cat return just a stream are in PR 256
Though one of these is a fix (using 'before' properly), the rest are timing issues that are more noticeable on slower computers.
ea0eb40
to
9bb79f6
Compare
9bb79f6
to
903022c
Compare
@noffle you mentioned that you had seen this behaviour before and fixed it. Thoughts? |
@diasdavid the linked test is showing as green to me. Still a problem? |
if (counter === 0) { | ||
ds.push(null) | ||
} else { | ||
setTimeout(canFinish, 100) |
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.
Why do we need this 100ms timeout loop?
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.
Cause getting the object is an Async operation and the underlying stream from the importer won't care to wait that we have pushed all the objects to the upper stream and emit them all and then the 'end' event. This avoid a racing condition.
deb75c8
to
20d880c
Compare
Builds on #261