[fixed]The .then() function is not compatible with Promises

[kornelski]

The faux .then() function added in 1.3 completely misses the point of promises and breaks even in simple cases.

request.get(someCorrectUrl)
    .then(function(){return 1;})
    .then(console.log.bind(console));

Should print 1, but throws an unhandled exception.

request.get('fail').then(function(){});

TypeError: reject is not a function

request.get('fail')
    .then(undefined, function(){throw Error();})
    .catch(function(){})

.then(...).catch is not a function

Please don't try to reimplement promises — just enable use of a correct library.

I suggest:

Request.prototype.then = function(resolve, reject) {
  var self = this;
  return new Promise(function(resolve, reject){
    self.end(function(err, res){
      if (err) reject(err); else resolve(res);
    });
  })
  .then(resolve, reject);
};

The code above assumes that promises are either supported natively (already the case in majority of browsers and even Node 0.12) or polyfilled by the user (which is a common practice, e.g. ES6 transpilers do it).

[rsolomo]

👍

[alex94cp]

+1

[kahnvex]

I'm pretty sure the intent was not to add a full on promise implementation. Promises A+ spec documents the use of thenables, objects with then functions. These can be used by libraries like Q to map the async task to a real promise. With Q you can do:

Q(thenable).then(...).fail(...).done();

https://promisesaplus.com/

[defunctzombie]

Yes, we did not actually intend to add full promise because I don't want to bloat the library for everyone for what I perceive to be a marginally (if not worse) api. We know this is not full promises and is not meant to be. If there is a solution you have that works better but is not requiring full promises to be present.

ping @matthewmueller

[kornelski]

I've suggested using global Promise constructor.

• It's only few lines of code, so I think it's not bloating superagent.
• It doesn't require superagent to ship with a promise library.

Is that not satisfactory?

[matthewmueller]

Yep, for additional context, the most recent PR aims to resolve #230. That was the only intent of the PR. It was not to add promise support.

Personally, if what you suggested works properly, all existing tests pass and you include a small catch implementation, I think that would be better that what was implemented in 1.3.

Also, if I'm not mistaken, your solution would require everyone to be running node 0.12 or io, right?

[kornelski]

It would not require node 0.12, but it would require a polyfill (e.g. you'd need babel or require('es6-promise').polyfill() on the IE6 of node versions).

[matthewmueller]

Yep, so it would require Node 0.12 or a polyfill. And the polyfill would need to be present on the browser implementation as well (for IE).

I tend to agree with @defunctzombie on this that it's a marginal API improvement for a non-trivial increase in file size (17.952 kB minified). Though I do agree and sympathize that the .then() implementation here may confuse folks coming from the promise world, so I'd be open to revisiting it with the constraints described above.

[kahnvex]

Yeah, please don't roll a Promise polyfill. I'd have to consider moving all my projects away from superagent at that point. It's a good amount of bloat, and we already have big projects using Q and superagent together. I'm sure a lot of other users would echo this sentiment. Thenables are fine, they should simply be documented as such.

[aredridel]

Heh. Definitely document the heck out of this. But at this point I'm moving away from projects that are using just thenables in favor of ones that use promises.

[kornelski]

@matthewmueller Where do you take that 17KB from? I think superagent bundling promises implementation would be a mistake, and a serious downside. Where necessary, I already have a promise polyfill in my code, and I don't want a second one. It's wasteful and could cause bugs (I've ran into such problems when libraries had their own copies of the Q library, but Q breaks in subtle ways if there's more than one copy).

I'm assuming that nobody would be interested in using the .then() method without using promises in their code, so it's safe to assume they must have a promise library already.

[kahnvex]

I'm assuming that nobody would be interested in using the .then() method without using promises in their code.

Are you assuming that when people use promise libraries, they add them to the global namespace? If so, that is sort of a silly assumption.

[matthewmueller]

Where do you take that 17KB from? I think superagent bundling promises would be a mistake, and a serious downside.

Here: https://github.com/jakearchibald/es6-promise/blob/master/dist/es6-promise.min.js

I'm assuming that nobody would be interested in using the .then() method without using promises in their code.

This is an example of where a .then() implementation is extremely useful: #709. Also resolves the discussion here: #230

[kornelski]

es6-promise currently overwrites native promise implementation due to (IMHO tiny and irrelevant) bugs in v8, so bundling of it in superagent would cause negative side-effects.

co is an interesting example, but not a good example of a case where promises are not available — yield and Promise are both part of ES6. Babel and Traceur compilers have Promise polyfills. Also versions of node that support yield also support Promises.

[matthewmueller]

co is an interesting example, but not a good example of a case where promises are not available

Yep, the point isn't about compatibility though, it's about file size. We wouldn't be having this discussion if either, Promises were implemented everywhere or we can implement an extremely light-weight version of promises that weighs in at like 1-3Kb minified. The PR implemented added 4kb unminified and since a lot of it is repeated, I suspect gzipped with the rest of the file, it basically goes away.

[kornelski]

I think you're wrongly assuming that you need to implement promises. The way I see it it's something you integrate with, not something you provide.

Bundling promises with superagent would be like bundling v8 with superagent, in case somebody wants to use the JavaScript library, but doesn't want to install a JavaScript VM.

[kahnvex]

I think you're wrongly assuming that you need to implement promises. The way I see it it's something you integrate with, not something you provide.

I think you are wrongly assuming that everyone who wants to use promises exposes a Promise global for other things to integrate with. Why is it necessary to break compatibility with users who don't make a Promise global? MDNs Promise documentation even shows you how to use a thenable to create a real promise. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/resolve

What are your reasons for pushing this? Is there some upside I'm missing that isn't already easily achievable? I can only think of down sides.

[matthewmueller]

Okay, so if I'm understanding what you're saying correctly, you're suggesting something along the lines of...

if ('undefined' != typeof Promise) {
  Request.prototype.then = function(resolve, reject) {
    var self = this;
    return new Promise(function(resolve, reject){
      self.end(function(err, res){
        if (err) reject(err); else resolve(res);
      });
    })
    .then(resolve, reject);
  };
}

If that's the case, then I don't really care much either way since it doesn't add much bulk and yield will work in environments that promises work. Not sure how @defunctzombie feels about that, but I'd welcome your PR.

[euwest]

I thought the original intent was to turn the request object into a "thenable" so any promise library with coercive functionality could turn it into a promise. So for native promises, it would look like:

var reqPromise = Promise.resolve(superagent.get('/whatever'))
  .then(something, otherThing)
  .then(null, blahBlah)
  ...

is this not the original intent? why force usage of native promises in browsers that support them?

[kornelski]

@kahnjw I think the concept of thenables exists primary for interoperability between Promise-compliant libraries.

It is possible to convert thenable to a fully-functional Promise, but it isn't convenient, and superagent's implementation is not functional enough to start a promise chain without explicit conversion.

I try to avoid thenables as much as I can, because they don't give guarantees that Promises have (e.g. that callbacks are run on the next tick) and can cause bugs.

In Superagent 0.13 case this happens to work thanks to casting:

doSomethingAsync().then(() => request.get()).then(res => nextStep());

but if you refactor the code to swap the first two functions, it'll unexpectedly break in a weird way:

request.get().then(res => doSomethingAsync()).then(()=> nextStep());

and while it's possible to fix it with an explicit Promise.resolve(), this isn't a usual thing to do in Promise-based code, and extra parenthesis are undesirable around fluent interfaces.

[euwest]

I just don't understand the desire to make this function reliant on a global Promise, simply to hide a Promise.resolve inside. There's so much room for pain, and the benefit seems so trivial.

[kornelski]

The global Promise is now part of ES6, so it isn't any worse than relying on global Array in ES5.

And the desire for it is to have a robust API without footguns. The faux promise stopgap thing:

• fails to support chaining
• fails to handle exceptions thrown from callbacks
• fails to defer execution to the next tick
• fails to handle optional callbacks
• etc etc.

I suppose you're not "sold" on Promises, but please understand that for somebody who's using them heavily Superagent's faux solution is not only insufficient, but even harmful.

v0.12 used without some glue code would clearly fail with "then is undefined", but v0.13 is now worst of the both worlds: without diligent wrapping in Promise.resolve() it may appear to work, but won't: exceptions will escape, callbacks won't be called in the right tick, and values won't be passed down the chain.

[kahnvex]

I think this is sacrificing compatibility for convenience. Doing this would break thenable coercion methods in most promise libraries on environments like Node 10, and on browsers like IE10, IE11 and Safari 6. I agree that your suggestion is cleaner, better, and preferable, but only marginally and if you assume everyone uses FireFox or Chrome. The downside is pretty bad for consumers relying on Promise libraries that don't polyfill Promise.

A thenable is a thing. It is in Promise A+ spec. Yes it is a stopgap solution until ES6 Promises are used everywhere. The javascript community is simply not there yet.

[kornelski]

It doesn't have to break compatibility if the faux solution is used as a fallback.

[thom4parisot]

Interesting discussion: I don't think there is any negative impact on using es6-promise server side.

The question is rather client-side, where we might want to include the Promise polyfill for convenience but as @pornel stated, we don't want to overbloat if people already rely on external polyfills.
Why not having two artefacts, one with and one without polyfills?

Usage wise, I think we are at a stage people are familiar with promises, especially since the rise of Node 4.x.

[WickyNilliams]

If people are averse to depending on a global Promise (I don't think that's a big deal since it's part of the language - not DOM - spec now), what about adding a mechanism for users to supply some factory function for producing promises? That way superagent has no implementation of promises itself (or half an implementation as it stands), and consumers can use their favourite promise implementation or the global Promise type.

Currently I find the then provided by superagent to not be useful. It gets painful/annoying having to wrap in a real promise every time, so instead I do the simple/lazy/bad/convenient (delete as appropriate) thing of monkeypatching Request.prototype.then to return a real promise. I suspect a lot of users do this also, or use one of the many libs which add in real promises.

[mienaikoe]

A thenable without chaining is no different than supplying a callback as an argument to the method. It serves no purpose other than to fool the developer into thinking it's a fully-implemented thenable. If you don't want to adhere to the promises/A+ spec, then remove the then function.

[kornelski]

This is fixed in 2.0.0

[nebulou5]

@pornel I've been reading your comments throughout. I'm with you all the way. These guys must not use Promises. I'm battling to get my superagent stuff promisified so to not manually wrap all of my api methods in promises. I don't see how anyone can argue against using global Promise either; It's an ECMA standard and the previous superagent implementation was pointless. Might as well use the callback.

[nebulou5]

Looks like version 2.0 is the answer.

[gaboesquivel]

so is it now possible to this ?

request
  .post('/api/pet')
  .send({ name: 'Manny', species: 'cat' })
  .set('X-API-Key', 'foobar')
  .set('Accept', 'application/json')
  .end()  
  .then(handleSuccess)
  .catch(handleError);

[kornelski]

No, .end() is the old world. Don't use it. .then() is a replacement for .end().

[gaboesquivel]

thanks