Skip to content

Enable promises.#153

Merged
tbranyen merged 2 commits intomasterfrom
enable-promises
Jul 4, 2014
Merged

Enable promises.#153
tbranyen merged 2 commits intomasterfrom
enable-promises

Conversation

@tbranyen
Copy link
Copy Markdown
Member

This request adds in support for optional Promises. This is super useful for control flow and will improve our install process and build process in future pulls.

@alessioalex
Copy link
Copy Markdown

I don't really see the point of including promises. Can't this be done in a separate module for example? I bet most of the people just want to use callbacks.

@eav
Copy link
Copy Markdown

eav commented Apr 30, 2014

+1 for promises

@alessioalex
Copy link
Copy Markdown

I'm not looking to start a debate, I am just suggesting to use what Node uses by default (callbacks), and leave this option as a wrapper into a separate module (named like promised-nodegit for ex). I think that would be best for everybody.

@tbranyen
Copy link
Copy Markdown
Member Author

@alessioalex Generally I would agree, except for the case of flow control. Promises are a fundamental building block and are coming to the language proper. This is a start to an unobtrusive and alternative approach to harmonize them with this library.

At the moment all asynchronous functions return undefined, a useless value. This PR attempts to leverage this known to always return a Promise. This is 100% optional. If you want to use callbacks and are happy with them, you don't need to modify your code. You don't have to know the return value even exists.

Without promises:

git.Repo.open(".git", function(err, repo) {

});

With promises:

git.Repo.open(".git").then(function(repo) {

});

@gnarf
Copy link
Copy Markdown

gnarf commented Apr 30, 2014

I'm just curious what happens with:

git.Repo.open(".git", function(err, repo) {
  // cuz why?
}).then(function(repo) {
  // because you happen to have both a callback and use it as a promise?
});

@tbranyen
Copy link
Copy Markdown
Member Author

Both would be triggered as it only augments the return value, not the original signature.

@tbranyen
Copy link
Copy Markdown
Member Author

tbranyen commented May 2, 2014

This has another added benefit of greatly simplifying the install process that is now based on Promises.

@alessioalex
Copy link
Copy Markdown

Since this doesn't remove the ability to use Nodegit with callbacks as before, I think it's a really good addition. I agree with you guys.

@tbranyen
Copy link
Copy Markdown
Member Author

tbranyen commented May 7, 2014

Blocked by #162 need more accurate isAsync information.

@tbranyen tbranyen changed the title WIP Enable promises. [WIP] Enable promises. Jun 9, 2014
@tbranyen tbranyen changed the title [WIP] Enable promises. Enable promises. Jul 4, 2014
@tbranyen
Copy link
Copy Markdown
Member Author

tbranyen commented Jul 4, 2014

No longer blocked since we have a 100% accurate definition file. This is ready to merge.

tbranyen added a commit that referenced this pull request Jul 4, 2014
@tbranyen tbranyen merged commit 61b5583 into master Jul 4, 2014
@tbranyen tbranyen deleted the enable-promises branch July 4, 2014 01:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants