-
Notifications
You must be signed in to change notification settings - Fork 16
Use promises instead of callbacks #15
Comments
+1 on that! 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? |
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:
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 |
nice, I like the pattern! |
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 |
...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
The text was updated successfully, but these errors were encountered: