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

ES6 Promise support #726

Closed
wants to merge 1 commit into from
Closed

ES6 Promise support #726

wants to merge 1 commit into from

Conversation

kornelski
Copy link
Contributor

Fixes #722

@kahnvex
Copy link

kahnvex commented Aug 19, 2015

This addresses my compatibility concerns by adding a thenable fallback, but is weird/unexpected from a behavioral standpoint.

Also, I'd suggest returning a promise from the .end() method if there is community support for it. Having the then method fire the request is arbitrary and weird. If the goal is to use real promises, we should use promises as they were intended - then methods should not perform actions other than to return a new promise. You could also add a new function .endPromise(), or something like that to address backward compatibility concerns.

@kornelski
Copy link
Contributor Author

I agree returning promise from end() would be better.

I also think it should cache the promise, so that var tmp = superagent.get(); tmp.then(); tmp.then() doesn't try to launch the request again.

@kornelski
Copy link
Contributor Author

BTW: the code for both node and browser happens to be identical. Do you have a preferred way for sharing/reusing it?

@euwest
Copy link

euwest commented Aug 19, 2015

I also think it should cache the promise, so that var tmp = superagent.get(); tmp.then(); tmp.then() doesn't try to launch the request again.

Yeah, which is further reason for end returning the promise and it(i.e. end) not being a part of then(maybe that's what you were saying?). That way you'd have:

var tmp = superagent.get().end();

tmp.then(); 
tmp.then();

@defunctzombie
Copy link
Contributor

I am gonna say it again. I do not care about supporting promises in this library. The .then hack was to make it a simple thenable and I was fine with that. If you want a promise then use a wrapper.

Yes, we need to document the .then call better.

benesch added a commit to WhoopInc/supertest-as-promised that referenced this pull request Sep 6, 2015
Since v1.1.0, SuperTest ships a version of SuperAgent with "thenable"
support; that is, a then method which provides minimal interop with
Promise libraries but does not provide full then semantics.

See ladjs/superagent#722 and ladjs/superagent#726 for
context. Since SuperTest's maintainer is entirely unwilling to support
promises, we'll continue to maintain this library for those who want
`.then()` to return a real promise.
@vire
Copy link

vire commented Sep 25, 2015

What's the status please, there is an closed PR, but in the latest version of superagent (v1.4.0 and node v0.12.7) is not working as expected (see #722).

@FoxxMD
Copy link

FoxxMD commented Dec 2, 2015

@vire It was closed but not merged. @defunctzombie said above that he will not support A+ promises

@vire
Copy link

vire commented Dec 2, 2015

@FoxxMD thanks for clarification. It's a pity indeed to not be complaint with some standard approach.

@euwest
Copy link

euwest commented Dec 9, 2015

It's a pity indeed to not be comlpaint with some standard approach.

Superagent is not a promises implementation, though. Request objects just have a then method to make it a little easier to coerce into a promise, given an actual implementation.

@niftylettuce
Copy link
Collaborator

Here's a promise implementation (uses ES6/ES7 async/await too, with ES6's new fetch() method) https://github.com/glazedio/frisbee.

@kornelski kornelski deleted the es6promise branch May 16, 2016 08:48
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.

None yet

7 participants