-
Notifications
You must be signed in to change notification settings - Fork 186
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
BREAKING CHANGE(core, schema): add app-wide skipThrowForStatus; adjust throwForStatus behavior #511
Conversation
@@ -14,7 +14,9 @@ const prepareRawResponse = (resp, request) => { | |||
skipThrowForStatus: request.skipThrowForStatus, | |||
}; | |||
const outResp = _.extend(resp, extendedResp, replaceHeaders(resp)); | |||
outResp.throwForStatus = () => throwForStatus(outResp) && undefined; | |||
outResp.throwForStatus = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added braces to arrow function, the && undefined
was a weird construct here.
} | ||
|
||
// eslint-disable-next-line yoda | ||
if (400 <= response.status && response.status < 600) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no-op change to add some clarity to these conditions (including breaking out if skipThrowForStatus
)
@@ -14,7 +14,9 @@ const prepareRawResponse = (resp, request) => { | |||
skipThrowForStatus: request.skipThrowForStatus, | |||
}; | |||
const outResp = _.extend(resp, extendedResp, replaceHeaders(resp)); | |||
outResp.throwForStatus = () => throwForStatus(outResp) && undefined; | |||
outResp.throwForStatus = () => { | |||
throwForStatus(outResp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This occurs to me that response.throwForStatus()
has a confusing behavior. Say a developer wants to handle error responses themselves, they might do this:
// example_1.js
const perform = async (z, bundle) => {
const response = await z.request({
url: '...',
skipThrowForStatus: true
});
respones.throwForStatus();
return response.data;
}
But this won't work. The response.throwForStatus()
line doesn't actually do anything because it also honors the skipThrowForStatus
flag. Instead, they have to unset skipThrowForStatus
to activate the response.throwForStatus()
call:
// example_2.js
const perform = async (z, bundle) => {
const response = await z.request({
url: '...',
skipThrowForStatus: true
});
response.skipThrowForStatus = false; // <--- 🤔
respones.throwForStatus();
return response.data;
}
This is awkward and confusing. I think in this PR we should make some changes so that the skipThrowForStatus
flag is only effective in the z.request
middleware. When a developer calls response.throwForStatus()
manually, it always throws for status, regardless of the value of skipThrowForStatus
.
To do that, first, we can move the definition of throwForStatus
from http-middlewares/after/throw-for-status.js
to prepare-response.js
as such:
// in prepare-response.js
outResp.throwForStatus = () => {
if (
response.status >= 400 &&
response.status < 600
) {
throw new errors.ResponseError(response);
}
};
Next, in http-middlewares/after/throw-for-status.js
, we honor skipThrowForStatus
and perhaps call response.throwForStatus()
:
// in http-middlewares/after/throw-for-status.js
const throwForStatus = (response) => {
if (!response.skipThrowForStatus) {
response.throwForStatus();
}
return response;
};
This way, the skipThrowForStatus
flag is only used in a middleware context, which is more intuitive. example_1.js
would work as expected.
Strictly speaking, this is a breaking change. But it's only breaking when a developer weirdly expects the response.throwForStatus()
line in example_1.js
to do nothing. Because now with the changes mentioned, the response.throwForStatus()
line in example_1.js
actually does something.
@xavdid Does that make sense? What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, that's fair! It is a pretty weird UX now that you mention it. It's interesting that we haven't gotten any feedback (afaik) about that usage.
In any case, I think your suggestion is a good one! I'll code it up tomorrow.
Ok! did some tidying up in there too; this should be ready. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes! Approving.
Since this is now technically a breaking change, maybe we should edit "This is a backwards-compatible change" in the PR description and the PR title?
Adds the ability to adjust the default
skipThrowForStatus
behavior across an entire app. Has no effect if a given request already declares anyskipThrowForStatus
value (see tests).Also adjusts
skipThrowForStatus
to only affect the middleware, not calling the function directly.