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

Is there a way to config the 4xx & 5xx statusCode as normal response but not error #1069

Closed
magicdawn opened this issue Sep 16, 2016 · 19 comments

Comments

@magicdawn
Copy link
Contributor

As the title presents.

I'm tired of writing code like this

const getDataAsync = co.wrap(function*(){
  let res;
  try {
    res = yield request.get('some_url')
  } catch(err) {
    if(!err.original && err.response) {
      res = err.response
    } else {
      throw err;
    }
  }
});

I have to test !err.original && err.response exists, and treat it as success response, I'm tired of writing this.
So I'm asking is there a way to config that and if not can this issue be a feature request?

@magicdawn
Copy link
Contributor Author

@focusaurus
Copy link
Contributor

I think this is one of those API decisions that could go either way. Either you write application code like that to treat responses with error status codes as successful responses or you change the superagent API to treat them as success, but then you have to write application code to look at the status code in the application and respond accordingly. For example, detecting a 404 with HTML in the body when you were expecting a 200 with JSON.

I personally wish the original superagent API was "any HTTP response is success" semantics, but it's not. This is something that would be such a breaking change I'd hesitate to change it without something as drastic as forking and renaming the project. I believe that has been discussed on github issues in the past so you might get some insight searching through closed issues for discussion.

I know of at least request-promise which has this behavior configurable:

By default, http response codes other than 2xx will cause the promise to be rejected. This can be overwritten by setting options.simple = false.

I would hesitate to make the core API configurable in this way, but I do understand your frustration.

@magicdawn
Copy link
Contributor Author

So since request-promise add configuration to this, how about superagent 🎉

@focusaurus
Copy link
Contributor

Well, if we do it, I'd rather do it with a different API like adding a
req.getResponse() method.

On Fri, Sep 16, 2016 at 8:37 AM Tao notifications@github.com wrote:

So since request-promise add configuration to this, how about superagent
🎉


You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#1069 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAdcSWMICCSs4_mdiiGXpj2Ud6o_nYSkks5qqqmlgaJpZM4J-1Q4
.

@kornelski
Copy link
Contributor

I think we could make the logic configurable by allowing user to supply a callback to judge whether the response is OK.

https://github.com/visionmedia/superagent/blob/v2.2.0/lib/node/index.js#L590

request.get().ok(res => res.status < 500).then()

@magicdawn
Copy link
Contributor Author

ping

@magicdawn
Copy link
Contributor Author

idea: global staticProperty

add a static property superagent.detectStatusCode

  • defaults to true, so we treat 4xx & 5xx response as error
  • when set to false, wo do not care the statusCode

@magicdawn
Copy link
Contributor Author

magicdawn commented Oct 10, 2016

idea: per req config

add a property or method to the superagent.Request class
e.g add Request.prototype.config

request
  .get('https://www.google.com.hk')
  .config({ detectStatusCode: false }) //  <- turn off detectStatusCode here
  .end((err, res) => {
    // ... blabla
  });

I'm 👍 on this too

@magicdawn
Copy link
Contributor Author

.ok is also ok

but add support short version like .ok(true) would be more nice

@magicdawn
Copy link
Contributor Author

ping

@keithamus
Copy link

request.get().ok(res => res.status < 500).then(…)

Very much works for me. I think if you want all to pass, request.get().ok(_ => true) is very succinct.

kornelski added a commit that referenced this issue Dec 12, 2016
Fixes #1069

Closes #1096 #1074
@kornelski kornelski mentioned this issue Dec 12, 2016
kornelski added a commit that referenced this issue Dec 13, 2016
Fixes #1069

Closes #1096 #1074
kornelski added a commit that referenced this issue Dec 13, 2016
Fixes #1069

Closes #1096 #1074
@Joshua-Swain
Copy link

I'm trying to use this function for to test an invalid post request. The function goes like this: chai.request(app).post(...).set(...).send(...).end((err, res) => {...});
I tried putting .ok(...) at every point in that chain, but I always received the error TypeError: chai.request(...).post(...).set(...).send(...).end(...).ok is not a function

I am expecting this to cause validation errors. Any advice on how to use this ok() feature?

@kornelski
Copy link
Contributor

This feature is in superagent. Are you using supertest perhaps?

@Joshua-Swain
Copy link

I'm pretty sure I'm using superagent, because I'm using chai out of the box.

@kornelski
Copy link
Contributor

kornelski commented Nov 21, 2017

Then request.post(url).ok(cb) should work. chai.request(app) doesn't look like a call to superagent, so maybe chai has a wrapper that doesn't expose it yet?

@focusaurus
Copy link
Contributor

I believe the chai-http plugin is in use here. That has superagent 3.7 which should have .ok I believe so not sure what's wrong.

@keithamus
Copy link

keithamus commented Nov 22, 2017

(chai-http maintainer here) We've yet to add support for this feature in a released version of chai-http. We'll be making a release soon. Thanks for everyone's input here.

@svicalifornia
Copy link

@keithamus Any update? What should we be doing until ok is added to chai-http?

@keithamus
Copy link

@svicalifornia we should take this off of the superagent tracker. Please track chaijs/chai-http#202 for updates.

BenjaminKowatsch pushed a commit to BenjaminKowatsch/InteractiveMedia that referenced this issue Jul 25, 2018
* chai-http currently does not support to properly use 'expect' to specify the bevahiour of calls returning 4XX or 5XX status codes
* chaijs/chai-http#75 (comment)
* ladjs/superagent#1069 (comment)
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

No branches or pull requests

6 participants