HTTP error handling regression from 3.5.2 to 3.6.0

[kevinoid]

With swagger-client@3.6.0 if a transport-level HTTP error occurs (e.g. host/port unreachable) it causes an internal error in swagger-client which suppresses the original HTTP error. The problem can be demonstrated with the following command:

node -e "require('swagger-client').http({url: 'http://localhost:1/'}).catch(console.error)"

With swagger-client@3.6.0 this will print TypeError: Cannot read property 'statusText' of undefined along with a stack trace. This is far less useful than the error produced by swagger-client@3.5.2, FetchError: request to http://localhost:1/ failed, reason: connect ECONNREFUSED 127.0.0.1:1.

I bisected the regression to commit 1e9f8f0 from #1261.

Thanks,
Kevin

[rg1]

I've looked into this. You're right, I've completely overlooked the fact that the fetch itself can already fail. I've fixed the issue in my commit d8d0ade plus a test that executes your sample. I'm not sure if it should rather be mocked, though. I also don't know if this needs to be a pull request or how to make this issue into one. Please advise.

[kevinoid]

I've looked into this. You're right, I've completely overlooked the fact that the fetch itself can already fail. I've fixed the issue in my commit d8d0ade plus a test that executes your sample.

Thanks @rg1!

I'm not sure if it should rather be mocked, though.

I usually use nock for mocking HTTP. It works well once setup, but I think it would take too much effort for this issue alone. I'd suggest changing 'http://localhost:1/' to 'http://invalid.invalid/' which is guaranteed to be unresolvable by RFC 2606. (I probably should have used that in the issue description.)

I also don't know if this needs to be a pull request or how to make this issue into one.

What I would do is amend commit d8d0ade to make the url change above and add Fixes: #1277 to your commit message then submit a pull request with Fixes: #1277 in the description. That way other contributors can review it before merging, when the PR is merged it will automatically close this issue, and anyone later reviewing the commit logs (e.g. bug hunting) will find a reference to this issue.

However, it's all about personal preferences and project conventions. I'm not sure how the Swagger team would prefer to do it. (I've only submitted an issue/PR or two, so I'm not heavily involved.) The above is just my general advice.

Thanks again,
Kevin

[shockey]

@kevinoid, thanks for reporting this and apologies for the trouble with your build!

@rg1, feel free to open a PR for this once you have a fix you're happy with. If you get stuck on the testing part, go ahead and open a PR and ping me there 😄

As for the conventions for PRs, our contribution guide over at Swagger-UI may help: https://github.com/swagger-api/swagger-ui/blob/master/CONTRIBUTING.md#making-pull-requests

[shockey]

fixed! 🎉

[kevinoid]

Thanks @shockey!

[rg1]

@shockey thanks for taking this. I got sick before I could update my patch.

[shockey]

@rg1, no worries! the commit you linked to helped a lot in shaping my fix.