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

Promise support #8

Merged
merged 6 commits into from
Dec 17, 2018
Merged

Promise support #8

merged 6 commits into from
Dec 17, 2018

Conversation

christav
Copy link
Contributor

@christav christav commented Dec 9, 2018

Implements #2.

Added transparent support for promise returning functions.
Added ambi.promise as a hook so that the promise implementation used can be replaced if desired.
Updated docs.

Hopefully I didn't break the automation; I'm working on Windows and a bunch of the NPM scripts don't work there, and I felt that fixing that problem (if you want to) is better in a separate PR.

@balupton
Copy link
Member

balupton commented Dec 10, 2018

Will need to be a major version change, as it makes the minimum node version now 0.12 instead of 0.8. I can do this. Which is why the CI tests failed.

If you are ok with this, can we dump the custom promise library support? People can just do customPromiseLibrary.resolve(ambi(…)) or ambi(…).then(customPromiseLibrary.resolve()) to convert it.

Otherwise, LGTM, excited to get this in there.

@christav
Copy link
Contributor Author

The custom library support is only necessary for supporting old environments (like Node 0.8 or really old browsers) that don't have a native promise implementation. If you're happy cutting that support then sure, happy to take it out.

@balupton
Copy link
Member

The custom library support is only necessary for supporting old environments (like Node 0.8 or really old browsers) that don't have a native promise implementation. If you're happy cutting that support then sure, happy to take it out. ...

@christav great, let's cut it. People can just use polyfills in that case anyway.

Sent with GitHawk

@christav
Copy link
Contributor Author

Hang on a sec, I think there's a fundamental misunderstanding on my part. I thought the whole purpose of this library was to consolidate the various sync and async calling patterns in to a single callback-based one. The example you specified above is predicated on ambi returning a promise, which my code does not do - and neither does yours for sync or async cases pre-PR. In fact I updated the docs because the ambi function explicitly returns null, not a value, so the docs didn't match the code. See index.js, line 97 for the code in question.

So I'm a little confused as to what you want the API to be here? In the meantime I'll take out the custom promise support in the meantime while we figure out the right api.

Removed support for plugging other promise libraries.
You can set the global Promise symbol if you really need to instead.
Bumped up minimum node version to 0.12 as a result.
Bumped major version of library due to requirements change.
@balupton
Copy link
Member

balupton commented Dec 10, 2018

I thought the whole purpose of this library was to consolidate the various sync and async calling patterns in to a single callback-based one.

Sorry for the confusion. That is correct. This does not need to return a promise.

Returning a promise can come in a later major version of ambi/taskgroup/kava/event-emitter-grouped, as it would require major additional changes to all of them, such as await ambi(…), await task.run(…), await taskgroup.run(…), await suite(…), await eeg.emitSerial(…).

Right now the biggest value we are wanting is to return promises to tasks/tests, which is what this PR will enable.

@christav
Copy link
Contributor Author

Updated PR, build is green. @balupton we good to merge?

@balupton
Copy link
Member

Will merge, and then will work on promise returns, as that seems to make most sense considering its API.

@balupton balupton merged commit c012675 into bevry:master Dec 17, 2018
@balupton
Copy link
Member

Released to v4

@christav christav deleted the promise-support branch December 17, 2018 04:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants