Skip to content
This repository has been archived by the owner on Dec 27, 2023. It is now read-only.

Use promises instead of callbacks #15

Open
danrr opened this issue Sep 4, 2017 · 4 comments
Open

Use promises instead of callbacks #15

danrr opened this issue Sep 4, 2017 · 4 comments

Comments

@danrr
Copy link

danrr commented Sep 4, 2017

...with a view to move to async/await in the near future.

Currently the template code is making heavy use of callback as is most of the oc ecosystem. Since this is a new project, I'm thinking it would be a good opportunity to start moving to Promises (and eventually async/await when support for node <8 isn't required anymore or through Babel). The callback APIs of the registry may be an issue but it may be possible to migrate this project with no changes on the registry side to start.

@nickbalestra and I had a quick chat about this, not sure who else to include

@nickbalestra
Copy link
Contributor

+1 on that!
As you said, as long as we keep the contract of public api in the callback format we should be able to move templates internals to promise, allowing for an easy path to async/await later on, and perhaps we could later expand this to the registry itself.

Would u suggest relying on libraries like bluebird or go with native Promises?

I will be happy to facilitate a PR in this direction, perhaps we could breake into multiple small PRs to refactor out each piece we use Async.js to start with, is this something you would like to help contribute?

@danrr
Copy link
Author

danrr commented Sep 13, 2017

Hey, happy you're on board!

One pattern a lot of libraries use when moving from callbacks to promises is to check for a callback and either execute it or return the promise, something like:

function publicMethod(callback) {
  const promise = doAsyncStuff();
  if (callback) { // can check for a function as the last param if the method signature is more complicated
    promise.then(result => callback(null, result), error => callback(error));
    return null;
  }  else {
    return promise;
  }
};

That would allow the use of promises everywhere internally, keep the current API, and make for a smooth transition to async/await for anyone calling the API.

As an example, the new version of pa11y https://github.com/pa11y/pa11y/blob/e28f89334cbf71c8ef22a497a9e84c0ffb9d3dd1/lib/pa11y.js#L20 uses this pattern

I generally prefer using native Promises since they're already there on most platforms, unless there's some very specific scenarios like limiting concurrency where Bluebird has better support.

I'm happy to contribute but I'm travelling until the end of the month and I won't be able to do that much until I'm back. Would it be ok to have a look at it then? Thanks

@nickbalestra
Copy link
Contributor

nice, I like the pattern!
No worries, there is no rush. I'll try to get a stable out in the meanwhile so that we can start from there once u back. Look forward working together on this.

@nickbalestra
Copy link
Contributor

I've tried to cleanup and abstract/reuse as much as possible from the base-templates packages. This should mean that starting from here it should be simpler and quicker and as we proceed further in the base-template packages all the other templates should also benefit from this

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants