New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal for Standardising API that can return buffered results vs Node.js Streams vs Pull Streams #126

Closed
diasdavid opened this Issue Mar 9, 2017 · 17 comments

Comments

Projects
None yet
4 participants
@diasdavid
Member

diasdavid commented Mar 9, 2017

tl;dr; this is a proposal to standardize the API definitions of function calls that may return buffered values, Node.js Streams or pull-streams.

I'll make this proposal and succinct as possible, with the hopes we can get around it asap.

Our current needs and requirements:

  • Users want to be able to consume data coming from calls (i.e: files.{cat, get, ls}) as a single result value. It is extremely convinient.
  • We know that buffering everything doesn't work for a big part of the cases.
  • We like to use pull-streams everywhere because it saves us from some Node.js hiccups and makes it extremely convenient between modules.
  • Users still to have access to Node.js Streams, since they are the most widely use.
  • We have a strong commitment to not breaking user-space, however, if this standardization proves to be popular and more user-friendly, I believe we can open an exception as long as we don't loose functionality (only some name changing), under the condition to have a smooth transition.

Essentially we have two options on the table

a) Use option arguments to specify what we are looking for

ipfs.files.cat(cid, {
  buffer: true         // buffer all the results and return a single value
  // PStream: true // return a Pull-Stream
  // NStream: true // return a Node.js Stream
}, callback)

b) Use different method names

ipfs.files.cat(cid, callback)
// or
ipfs.files.catPStream(cid)
// or
ipfs.files.catNStream(cid)

This option has a couple of advantages:

  • interpreter friendly - by having a method for each type, it will enable V8 to optimize it better
  • comparing to what we have today, it will enable the functions that return streams to return streams, instead of the 'callback with a stream'. Enabling things like ipfs.files.catNStream(cid).pipe(process.stdout) without going into callback. (The reason why we had to return a stream in a callback, was because of Promises, but that stays on ipfs.files.cat only.

Once this proposal goes forward we need to:

  • Update the API definitions
  • Update the tests
  • Update the implementations
  • Update the internals that exposes pull-streams with something like getStream, to be getPStream.

@diasdavid diasdavid referenced this issue Mar 9, 2017

Merged

More DAG API goodness #123

3 of 3 tasks complete
@diasdavid

This comment has been minimized.

Show comment
Hide comment
@diasdavid

diasdavid Mar 9, 2017

Member

@ipfs/javascript-team I believe you all might want to chime on this.

Member

diasdavid commented Mar 9, 2017

@ipfs/javascript-team I believe you all might want to chime on this.

@dignifiedquire

This comment has been minimized.

Show comment
Hide comment
@dignifiedquire

dignifiedquire Mar 9, 2017

Member

👍 for proposal b from my side, not sure about the names and naming is hard so go ahead with these :)

Member

dignifiedquire commented Mar 9, 2017

👍 for proposal b from my side, not sure about the names and naming is hard so go ahead with these :)

@diasdavid diasdavid added the ready label Mar 9, 2017

@nicola

This comment has been minimized.

Show comment
Hide comment
@nicola

nicola Mar 9, 2017

Member

I am for option b,

what about

ipfs.files.cat(cid, callback)
// or
ipfs.files.catPullStream(cid)
// or
ipfs.files.catStream(cid)

?

Member

nicola commented Mar 9, 2017

I am for option b,

what about

ipfs.files.cat(cid, callback)
// or
ipfs.files.catPullStream(cid)
// or
ipfs.files.catStream(cid)

?

@diasdavid

This comment has been minimized.

Show comment
Hide comment
@diasdavid

diasdavid Mar 10, 2017

Member

@dignifiedquire @nicola thank you for the feedback!

With regards to the naming. I don't have strong feelings about 'catPStream' and 'catNStream', I kind of like @nicola suggestion. The only that feels weird to me from all the options shared was: 'catPull' without the 'Stream' keyword.

Member

diasdavid commented Mar 10, 2017

@dignifiedquire @nicola thank you for the feedback!

With regards to the naming. I don't have strong feelings about 'catPStream' and 'catNStream', I kind of like @nicola suggestion. The only that feels weird to me from all the options shared was: 'catPull' without the 'Stream' keyword.

@VictorBjelkholm

This comment has been minimized.

Show comment
Hide comment
@VictorBjelkholm

VictorBjelkholm Mar 10, 2017

Member

Yeah, @nicola's suggestion makes sense to me too. Divide the methods would be the most beneficial and least confusing (more arguments/options for a function, the less intuitive). Also, regarding the naming, using full names like catPullStream and catStream makes it easier to assume or remember what is returned, rather than using shortnames that can be confusing.

Member

VictorBjelkholm commented Mar 10, 2017

Yeah, @nicola's suggestion makes sense to me too. Divide the methods would be the most beneficial and least confusing (more arguments/options for a function, the less intuitive). Also, regarding the naming, using full names like catPullStream and catStream makes it easier to assume or remember what is returned, rather than using shortnames that can be confusing.

@diasdavid

This comment has been minimized.

Show comment
Hide comment
@diasdavid

diasdavid Aug 1, 2017

Member

We need to provide this change the sooner the better. Streams are still confusion for devs.

One thing to consider is to s/catStream/catReadableStream since we will use in fact https://www.npmjs.com/package/readable-stream and not Node.js core streams to ensure compatibility and stability across Node.js versions.

We also should consider aliases

  • catReadableStream -> catRS
  • catPullStream -> catPS
Member

diasdavid commented Aug 1, 2017

We need to provide this change the sooner the better. Streams are still confusion for devs.

One thing to consider is to s/catStream/catReadableStream since we will use in fact https://www.npmjs.com/package/readable-stream and not Node.js core streams to ensure compatibility and stability across Node.js versions.

We also should consider aliases

  • catReadableStream -> catRS
  • catPullStream -> catPS
@dignifiedquire

This comment has been minimized.

Show comment
Hide comment
@dignifiedquire

dignifiedquire Sep 4, 2017

Member

The short names are reeeeaally hard to distinguish with P and R looking almost the same, so I think we should go for the long versions.

Member

dignifiedquire commented Sep 4, 2017

The short names are reeeeaally hard to distinguish with P and R looking almost the same, so I think we should go for the long versions.

@dignifiedquire

This comment has been minimized.

Show comment
Hide comment
@dignifiedquire

dignifiedquire Sep 4, 2017

Member

Not sure ReadableStream is good though, given that they sometimes might be duplex or other kind of streams, so could be more confusing.

Member

dignifiedquire commented Sep 4, 2017

Not sure ReadableStream is good though, given that they sometimes might be duplex or other kind of streams, so could be more confusing.

@dignifiedquire

This comment has been minimized.

Show comment
Hide comment
@dignifiedquire

dignifiedquire Sep 4, 2017

Member

Maybe we could do

// callback
files.cat(cid, callback)
// node stream
files.cat(cid)
// pull streams
files.catPull(cid)

It's easy to detect argument count and this might make it a bit less awkward to find correct names

Member

dignifiedquire commented Sep 4, 2017

Maybe we could do

// callback
files.cat(cid, callback)
// node stream
files.cat(cid)
// pull streams
files.catPull(cid)

It's easy to detect argument count and this might make it a bit less awkward to find correct names

@diasdavid

This comment has been minimized.

Show comment
Hide comment
@diasdavid

diasdavid Sep 4, 2017

Member

Ok, let's ship this for js-ipfs 0.27.0. The resolution is:

Expose 3 higher level calls for streaming methods (cat used as example):

  • cat(cid, function (err, bufferedFile) {}) -> Promise - Support Callback or Promise API with promisify-es6
  • catReadableStream(cid) -> [ReadableStream](https://www.npmjs.com/package/readable-stream)
  • catPullStream(cid) -> [PullStream](http://npmjs.org/pull-stream)

This will unlock use cases with perf improvements as shown in ipfs/js-ipfs#988

Member

diasdavid commented Sep 4, 2017

Ok, let's ship this for js-ipfs 0.27.0. The resolution is:

Expose 3 higher level calls for streaming methods (cat used as example):

  • cat(cid, function (err, bufferedFile) {}) -> Promise - Support Callback or Promise API with promisify-es6
  • catReadableStream(cid) -> [ReadableStream](https://www.npmjs.com/package/readable-stream)
  • catPullStream(cid) -> [PullStream](http://npmjs.org/pull-stream)

This will unlock use cases with perf improvements as shown in ipfs/js-ipfs#988

@dignifiedquire

This comment has been minimized.

Show comment
Hide comment
@dignifiedquire

dignifiedquire Sep 4, 2017

Member

Oh I missed promises in my proposal..

Member

dignifiedquire commented Sep 4, 2017

Oh I missed promises in my proposal..

@diasdavid

This comment has been minimized.

Show comment
Hide comment
@diasdavid

diasdavid Sep 4, 2017

Member

Pushed at the same time. Reviewing your proposal, @dignifiedquire There is a reason why it doesn't work. files.cat(cid) wouldn't be able to return a Node.js Stream because it would have to return a promise.

The other reason to call it ReadableStream and not Node.js Stream is that somehow ReadableStream and Node.js Stream have diverged (and we have caught bugs because of that, i.e isStream vs is-stream). So let's talk of ReadableStream as the Type/Class https://www.npmjs.com/package/readable-stream

Member

diasdavid commented Sep 4, 2017

Pushed at the same time. Reviewing your proposal, @dignifiedquire There is a reason why it doesn't work. files.cat(cid) wouldn't be able to return a Node.js Stream because it would have to return a promise.

The other reason to call it ReadableStream and not Node.js Stream is that somehow ReadableStream and Node.js Stream have diverged (and we have caught bugs because of that, i.e isStream vs is-stream). So let's talk of ReadableStream as the Type/Class https://www.npmjs.com/package/readable-stream

@dignifiedquire

This comment has been minimized.

Show comment
Hide comment
@dignifiedquire

dignifiedquire Sep 4, 2017

Member

I wonder if it wouldn't be better to have sth like

  • require('ipfs/promise')
  • require('ipfs/pull')

that way we could keep smaller bundles and easier names

Member

dignifiedquire commented Sep 4, 2017

I wonder if it wouldn't be better to have sth like

  • require('ipfs/promise')
  • require('ipfs/pull')

that way we could keep smaller bundles and easier names

@diasdavid

This comment has been minimized.

Show comment
Hide comment
@diasdavid

diasdavid Sep 4, 2017

Member

re: #126 (comment)

Eventually, we will have ES6 Imports and I prefer waiting for that then yet having to explain to users that they have to require ipfs in different ways to get a different API.

Member

diasdavid commented Sep 4, 2017

re: #126 (comment)

Eventually, we will have ES6 Imports and I prefer waiting for that then yet having to explain to users that they have to require ipfs in different ways to get a different API.

@dignifiedquire

This comment has been minimized.

Show comment
Hide comment
@dignifiedquire

dignifiedquire Sep 4, 2017

Member

I would suggest then going with cat, catStream and catPullStream

Member

dignifiedquire commented Sep 4, 2017

I would suggest then going with cat, catStream and catPullStream

@diasdavid

This comment has been minimized.

Show comment
Hide comment
@diasdavid

diasdavid Oct 22, 2017

Member

Work happening here #162

Member

diasdavid commented Oct 22, 2017

Work happening here #162

@diasdavid

This comment has been minimized.

Show comment
Hide comment
@diasdavid

diasdavid Nov 20, 2017

Member

🚢

Member

diasdavid commented Nov 20, 2017

🚢

@diasdavid diasdavid closed this Nov 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment