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

Support async transformFn when it returns Promise #2

Merged
merged 3 commits into from
Oct 19, 2016

Conversation

zbigg
Copy link

@zbigg zbigg commented Oct 18, 2016

This enables to use gulp-transform with async workflows using Promises.

Example usage:

pipe(transform(function(content) {
     return somedb.get(content); // returns Promise<String>
})

Note, test cases are not yet updated, I will add test cases if there is interest in this PR in this or other form.

@zbigg
Copy link
Author

zbigg commented Oct 18, 2016

Now i see, that this module supports node 0.10 which doesn't support promises. Not sure if anone needs to support this old version of node, esp it's development env (it goees EOL very soon - E10/2016: https://github.com/nodejs/LTS).

I would avoid any dependency on Promise shim ... I'll gladly change this PR to support callback-style async, but api for this is not straightforward

@mcmath
Copy link
Owner

mcmath commented Oct 19, 2016

Thanks for the PR. I expected someone might want this, but I didn't expect them to do the work for me. I'll take a look at it and hopefully merge it shortly.

I don't think we need to support callback-style async; I agree the API would be problematic. But, I would like to keep supporting Node 0.10 for now; there seems to be a surprising number of people still shackled to that old thing. But I can take care of that. I'll likely just merge your changes and then make a few tweaks of my own.

Thanks again.

@zbigg
Copy link
Author

zbigg commented Oct 19, 2016

Thanks for feedback.
FYI, I've already fixed few problems (unbound this) and made tests passing.
Will update PR later today.

On 19 Oct 2016 2:50 a.m., "Akim McMath" notifications@github.com wrote:

Thanks for the PR. I expected someone might want this, but I didn't expect
them to do the work for me. I'll take a look at it and hopefully merge it
shortly.

I don't think we need to support callback-style async; I agree the API
would be problematic. But, I would like to keep supporting Node 0.10 for
now; there seems to be a surprising number of people still shackled to that
old thing. But I can take care of that. I'll likely just merge your changes
and then make a few tweaks of my own.

Thanks again.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#2 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABcA1qaqgWLmPQo5wW6s5nED7HV1uXPYks5q1WlvgaJpZM4KZnjy
.

@mcmath mcmath merged commit f78baac into mcmath:master Oct 19, 2016
mcmath added a commit that referenced this pull request Oct 19, 2016
Merges #2
Merges #3
mcmath added a commit that referenced this pull request Oct 19, 2016
This commit adds support for asynchronous transformations.

## Community Contributions

 * #2 by @zbigg. Adds asynchronous support.

## Feautres

 * Asyncronous support by returning a promise

## Maintenance

 * Adds es6-promise dependency for compatibility with Node 0.10 to 0.12
 * Update outdated dependencies
 * Update README.md with async documentation
@mcmath
Copy link
Owner

mcmath commented Oct 19, 2016

Thanks a lot for this PR. I merged it, tweaked it to get the tests working, and now it's published as v1.1.0.

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