Skip to content

YAPPR - Yet Another Promise PR #262

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

Merged
merged 2 commits into from
Jun 28, 2016

Conversation

jamestalmage
Copy link

this simplifies the initial implementation of Promise support by @gero3, and allows for pluggable promise implementations. If a global Promise function exists, it will use that. Optionally, users can supply a promise implementation via config.Promise:

var GitHubApi = require('github');

var gh = new GitHubApi ({
  version: '3.0.0',
  Promise: require('bluebird')
});

This keeps the dependency list small, and gives people the freedom to choose their favorite Promise library (there have been at least 3 different libraries recommended in the issues so far). Most promise libraries are pretty huge, and now that I have this working in the browser, I would be concerned about forcing large dependencies on people. This also has potential benefits for browser frameworks with existing promise implementations (I'm thinking specifically of angular.js $q).

see #258, #232

gero3 and others added 2 commits June 9, 2015 06:02
this simplifies the initial implementation of Promise support by
@gero3, and allows for pluggable promise implementations. If a
global `Promise` function exists, it will use that. Optionally,
users can supply a promise implentation via `config.Promise`.

this keeps the dependency list small, and gives people the freedom
to choose their favorite Promise library.
@hofan41
Copy link

hofan41 commented Oct 15, 2015

Just stumbled in, it appears that the lead maintainer for this project has gone on hiatus, and I am itching for this feature.

When can we expect @mikedeboer to be back and responding? If not, should a new fork be created?

@2color
Copy link

2color commented Mar 10, 2016

@jamestalmage There's effort to merge some of the PRs in this fork:
https://github.com/kaizensoze/node-github

I'd suggest to make the PR there.

@jamestalmage
Copy link
Author

I'm not going to have time for that in the near future.

@kaizensoze kaizensoze added feature and removed feature labels Jun 9, 2016
@codydaig
Copy link

👍 for this! I currently just wrap the calls in helper functions that return a new promise and this would simplify the code a lot!

@kaizensoze
Copy link

@codydaig Couldn't you do something like

var Promise = require("bluebird");
var GitHubApi = Promise.promisifyAll(require("github"));

@codydaig
Copy link

@kaizensoze Primarily, I don't like the naming convention of adding Async to the end of every function name. Also, I don't like how it duplicates every method in the API, taking up twice as much space in memory. So, it's really just a personal preference.

@kaizensoze
Copy link

@codydaig Fair enough

@kaizensoze kaizensoze merged commit 49fa098 into octokit:master Jun 28, 2016
@thejohnfreeman
Copy link

Thanks for merging!

@gr2m
Copy link
Contributor

gr2m commented Oct 25, 2017

Hey folks, it's been a year and things have changed. We plan to drop support for the custom Promise option altogether. If that causes any trouble for you, could you please let me know at #622?

Thanks :)

@octokit octokit locked and limited conversation to collaborators Oct 25, 2017
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.

8 participants