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

WIP http cat #227

Merged
merged 3 commits into from
May 21, 2016
Merged

WIP http cat #227

merged 3 commits into from
May 21, 2016

Conversation

nginnever
Copy link
Member

This is currently using the unixfs-engine event emitter but will be switching to a full duplex stream in the unixfs-engine.exporter soon

@jbenet jbenet added the status/in-progress In progress label May 17, 2016
@dignifiedquire
Copy link
Member

What I thought when I read ...cat.. in my notifications

giphy 3

@nginnever
Copy link
Member Author

Haha let's hope for none of this.

@daviddias
Copy link
Member

daviddias commented May 18, 2016

Awesomesauce @nginnever :)

Just to confirm, are you are top of converting the exporter in unixfs to a duplex stream?

@nginnever
Copy link
Member Author

@diasdavid yeah so this PR will update with the exporter.

})
})

it('returns a stream', (done) => {
Copy link
Member

Choose a reason for hiding this comment

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

Buffer is not a stream

}).code(500)
}
stream.on('data', (data) => {
return reply(data.stream)
Copy link
Member

Choose a reason for hiding this comment

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

This could lead to strange issues when multiple events are emitted from the stream.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this will be okay. The exporter stream in object mode only emits one data event per file. And cat can only request one file to my knowledge. If we allow at some point for cat to take multiple hashs and return multiple file objects then we could have a problem with this. Thoughts?

Copy link
Contributor

@hackergrrl hackergrrl May 21, 2016

Choose a reason for hiding this comment

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

It probably will be okay, but I think @dignifiedquire's point was about approaching the mechanism defensively: just because today we can reason out that this should never happen doesn't mean future changes by future authors will always know+remember to hold this requirement. "It's a stream after all, and you can have many data events on streams.."

@nginnever
Copy link
Member Author

this will need PR to be merged

if (res) {
res.on('file', (data) => {
res.on('data', (data) => {
data.stream.pipe(process.stdout)
})
Copy link
Member

Choose a reason for hiding this comment

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

I believe this console.log should not be here and the if (res) is not necessary.

@daviddias daviddias merged commit 695a536 into master May 21, 2016
@daviddias daviddias deleted the cat/http branch May 21, 2016 10:04
@jbenet jbenet removed the status/in-progress In progress label May 21, 2016
@hackergrrl
Copy link
Contributor

I think @dignifiedquire had an unresolved line comment on this?

@dignifiedquire
Copy link
Member

@noffle it will be fine as soon as #253 is merged

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.

5 participants