Skip to content
This repository has been archived by the owner on Mar 10, 2020. It is now read-only.

refactor: convert config API to async await #1155

Merged
merged 9 commits into from
Nov 14, 2019

Conversation

alanshaw
Copy link
Contributor

@alanshaw alanshaw commented Nov 12, 2019

BREAKING CHANGE: Errors returned from request failures are now all HTTPErrors which carry a response property. This is a Response that can be used to inspect all information relating to the HTTP response. This means that the err.status or err.statusCode property should now be accessed via err.response.status.

@alanshaw alanshaw marked this pull request as ready for review November 13, 2019 21:39
src/config/set.js Outdated Show resolved Hide resolved
expect(err.message).to.eql('boom')
expect(err.status).to.eql(500)
expect(err.response.status).to.eql(500)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Breaking change? I guess errors aren't really part of the spec other than 'you will get an error'.

Copy link
Contributor Author

@alanshaw alanshaw Nov 14, 2019

Choose a reason for hiding this comment

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

Yes, it is breaking. I've updated the PR description with the info and will include it when merging.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great. One small thing - the description says err.status has changed but it's err.statusCode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well this test was looking for .status but others are looking for .statusCode...I've added both to the breaking change message.

@alanshaw alanshaw merged commit 621973c into master Nov 14, 2019
@alanshaw alanshaw deleted the refactor/config-async-await branch November 14, 2019 13:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants