Skip to content
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

Promises based API #360

Closed
wilhelmmatilainen opened this issue Jun 15, 2015 · 9 comments
Closed

Promises based API #360

wilhelmmatilainen opened this issue Jun 15, 2015 · 9 comments

Comments

@wilhelmmatilainen
Copy link

@wilhelmmatilainen wilhelmmatilainen commented Jun 15, 2015

Webtorrent is something new.
Compared to events, promises are something new in node.js.

How about writing webtorrent using bluebird promises?
https://github.com/petkaantonov/bluebird#what-are-promises-and-why-should-i-use-them

@madd512

This comment has been minimized.

Copy link

@madd512 madd512 commented Jun 15, 2015

let torrent = yield client.add('leaves of grass.torrent')
let book = yield torrent.getBuffer('leaves of grass.epub')
read(book)

Yeah, a promise API would be nice.

@wilhelmmatilainen

This comment has been minimized.

Copy link
Author

@wilhelmmatilainen wilhelmmatilainen commented Jun 15, 2015

Or like this

client.download('<magnet-uri>')
    .then(seed)
    .catch(log.error);
@feross

This comment has been minimized.

Copy link
Member

@feross feross commented Jun 16, 2015

I don't use promises, but I'm willing to consider adding promises to WebTorrent if it's backwards compatible and doesn't add a ton of extra code. Rather than include a library like bluebird, I'd prefer to just use the built-in platform Promises, if they exist.

PR welcome!

@feross feross changed the title Bluebird promises Promises based API Jun 16, 2015
@eabay

This comment has been minimized.

Copy link

@eabay eabay commented Jun 22, 2015

jwalton/node-promise-breaker can be a good fit for the implementation.

@wilhelmmatilainen

This comment has been minimized.

Copy link
Author

@wilhelmmatilainen wilhelmmatilainen commented Jun 23, 2015

Why bluebird:
http://programmers.stackexchange.com/questions/278778/why-are-native-es6-promises-slower-and-more-memory-intensive-than-bluebird

How

var promise = require('bluebird');

function oldFunction(a, b, callback) {
  if(a === b)
    callback(); // Success
  else
    callback(new Error('a does not equal to b')); // Fail
}

function newFunction(a, b) {
  return new promise(function(success, fail) {
    oldFunction(a, b, function(error) {
      if(error)
        return fail(error);
      return success();
    });
  });
}

function someFunctionUsingTheNewFunction(a, c) {
  return newFunction(a, c+1);
}

function letsChainStuff() {
  return newFunction(1, 1)
    .then(function() {
      return newFunction(2, 2);
    }).then(function() {
      return newFunction(3, 3);
    }).then(function() {
      return newFunction(4, 4);
    }).then(function() {
      return newFunction(1, 333); // This will fail
    });
}

letsChainStuff()
.then(function() {
  console.log('all done');
}).catch(function(error) {
  console.error(error); // Some nasty error happened
});

Notice that someFunctionUsingTheNewFunction?
There's no additional success or fail check. Why?
The promise it returns contains the information.

If you won't return a promise, a chain will continue directly to the next function.
When you return a promise, it's waited for before continuing to the next function.

With bluebird promises it's also possible and very easy to make something execute in _series_ or in _parallel_.

@feross

This comment has been minimized.

Copy link
Member

@feross feross commented Jun 28, 2015

Sorry, we're not bundling bluebird. That's 161KB of additional JavaScript that browser users would need to download.

@feross

This comment has been minimized.

Copy link
Member

@feross feross commented Jul 6, 2015

I think the best way to get promises into webtorrent core is for someone who uses promises regularly (not me) to publish a new package webtorrent-promise as an experiment to show what the API would work like. If it's nice, we can merge it into core.

@feross feross closed this Jul 6, 2015
@FluorescentHallucinogen

This comment has been minimized.

Copy link
Contributor

@FluorescentHallucinogen FluorescentHallucinogen commented Apr 8, 2018

In 2018 the JS promises are supported in the browsers and Node.js natively.

@feross

to show what the API would work like

For example

client.add(magnetURI).then(torrent => {
  console.log(torrent.infoHash);
}).catch(error => {
  console.error(error);
})

instead of

client.add(magnetURI, function(torrent) {
  console.log(torrent.infoHash);
})

and

client.seed(files).then(torrent => {
  console.log(torrent.infoHash);
}).catch(error => {
  console.error(error);
})

instead of

client.seed(files, function(torrent) {
  console.log(torrent.infoHash);
})

and so on…

@feross

I'm willing to consider adding promises to WebTorrent if it's backwards compatible

See how Firebase team has solved this problem: https://firebase.googleblog.com/2016/01/keeping-our-promises-and-callbacks_76.html. They added promises, but keep callbacks for backward compatibility. All functions in Firebase API now accept both Promise and callback-style methods.

@feross

This comment has been minimized.

Copy link
Member

@feross feross commented May 15, 2018

The ecosystem around Promises has matured quite a bit since I closed this issue. It's important to consider how to add Promises to the codebase without making the control flow hard to follow. I think I'd rather remove callbacks entirely to simplify maintenance, reduce the amount of code we need to test, etc.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.