Skip to content

Changed end() to return a Promise when called without arguments.#163

Closed
jussi-kalliokoski wants to merge 1 commit intoforwardemail:masterfrom
jussi-kalliokoski:end-promise
Closed

Changed end() to return a Promise when called without arguments.#163
jussi-kalliokoski wants to merge 1 commit intoforwardemail:masterfrom
jussi-kalliokoski:end-promise

Conversation

@jussi-kalliokoski
Copy link
Copy Markdown

This is a pretty handy thing to have, especially with Mocha, allowing you to go from this (example from supertest's own tests):

it('should support nested requests', function(done){
  var app = express();
  var test = request(app);

  app.get('/', function(req, res){
    res.send('supertest FTW!');
  });

  test
  .get('/')
  .end(function(){
    test
    .get('/')
    .end(function(err, res){
      (err === null).should.be.true;
      res.should.have.status(200);
      res.text.should.equal('supertest FTW!');
      done();
    });
  });
});

to this:

it('should support nested requests', function(){
  var app = express();
  var test = request(app);

  app.get('/', function(req, res){
    res.send('supertest FTW!');
  });

  return test
  .get('/')
  .end()
  .then(function (res) {
    return test
    .get('/')
    .end();
  }).then(function (res) {
    res.should.have.status(200);
    res.text.should.equal('supertest FTW!');
  });
});

Or, in a simpler case, the boilerplate reduction is even more obvious:

it('should do something', function (done) {
  request(app)
  .get('/')
  .expect(200)
  .end(function (err, res) {
    if (err) { return done(err); }
    done();
  });
});

vs

it('should do something', function(){
  return request(app)
  .get('/')
  .expect(200)
  .end();
});

Do note that this depends on ES6 Promises so it works only on node 0.11.x and later, but the es6-promise polyfill can be used for older versions.

@gjohnson
Copy link
Copy Markdown
Contributor

Let's keep this around until we can get superagent updated to keep them somewhat similar. It needs lots of work though, so might be a while :-/

@jakecraige
Copy link
Copy Markdown

@gjohnson What's the status of this? This would be a great addition, specifically for use with mocha

@gaastonsr
Copy link
Copy Markdown

👍

@jclem
Copy link
Copy Markdown

jclem commented Dec 11, 2014

I desperately needed this and hacked around it with this:

var Bluebird  = require('bluebird'); // Promise lib
var Test      = require('supertest/lib/test');
Test.prototype.end = Bluebird.promisify(Test.prototype.end);

@gaastonsr
Copy link
Copy Markdown

Exists a library for this for anyone interested.

@avimar
Copy link
Copy Markdown

avimar commented Mar 19, 2015

Hmm. I've also been using supertest-as-promised, which doesn't even require end() to return a promise, but I don't really know how hacky it is.
My main async calls - stripe and ORM (waterline) provide promises built-in, it would be great if my main API tester supertest also supported promises natively, since my test runner mocha is already able to handle them.

That said, supertest-as-promised seems to function great!

@arikon
Copy link
Copy Markdown

arikon commented Mar 20, 2015

It is not very hacky — you just need to add then() method to supertest's Test, that will accept callback and errback, that will call end(). And that's it.

@arikon
Copy link
Copy Markdown

arikon commented Mar 20, 2015

This is an example for Q library:

var q = require('q');

Test.prototype.then = function(onFulfilled, onRejected) {
    var self = this;
    return q.Promise(function(resolve, reject) {
        self.end(function(err, res) {
            if (err) {
                return reject(err);
            }
            resolve(res);
        });
    }).then(onFulfilled, onRejected);
};

With that fix you could use supertest like this (in mocha tests, for example)

var supertest = require('supertest');
it('should be ok', function() {
    return supertest(app).get('/').expect(200);
});

@gaastonsr
Copy link
Copy Markdown

I can confirm supertest-as-promised is a tiny wrap around supertest.

@JSteunou
Copy link
Copy Markdown

JSteunou commented Jan 7, 2016

👍 for this, would be great to see it in mainline project even if "promisified" solutions exist.

@r1b
Copy link
Copy Markdown

r1b commented Jan 20, 2016

+1

@grindfuk
Copy link
Copy Markdown

bump

@tilfin
Copy link
Copy Markdown

tilfin commented Mar 9, 2016

+1

Comment thread lib/test.js
var end = Request.prototype.end;

if (!fn) {
return new Promise(function (resolve, reject) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should probably be using any-promise.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be aware that the .pipe() method of Superagent expects .end() to return the instance itself and not a promise. IE. this breaks .pipe(). I managed to "promisify" .end() like shown below.

var originalEnd = Test.prototype.end;

Test.prototype.end = function (callback) {
  var deferred = Promise.defer();

  originalEnd.call(this, function (err, res) {
    if (callback) {
      callback(err, res);
    }

    if (err) {
      deferred.reject(err);
    } else {
      deferred.resolve(res);
    }
  });

  this.then = deferred.promise.then.bind(deferred.promise);
  this.catch = deferred.promise.catch.bind(deferred.promise);

  return this;
};

@gabzim
Copy link
Copy Markdown

gabzim commented Jun 6, 2016

+1

2 similar comments
@yf-hk
Copy link
Copy Markdown

yf-hk commented Jun 27, 2016

+1

@jeonghwan-kim
Copy link
Copy Markdown

+1

@robinvdvleuten
Copy link
Copy Markdown

I've opened a new PR for this functionality which uses native promises by default and the .then() method as suggested by @arikon

@rimiti
Copy link
Copy Markdown
Contributor

rimiti commented Apr 23, 2018

@jussi-kalliokoski can you resolve these conflicts ? Could you please take in consideration the @PlasmaPower and @robinvdvleuten comments.

@mikelax
Copy link
Copy Markdown
Contributor

mikelax commented May 17, 2018

@rimiti I think this PR can be closed at least in favor of #380
We should separately decide what we need beyond the Promise support already in superagent.

@rimiti rimiti closed this May 24, 2018
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.