-
Notifications
You must be signed in to change notification settings - Fork 24
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
Add ResponseSizeTooLarge Error #3020
Conversation
sam-codaio
commented
Jul 16, 2024
- add response too large error
- bump version to pre-release
* | ||
* @hidden | ||
*/ | ||
export class ResponseSizeTooLargeError extends Error { |
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.
Do we want to try to include any information here that would help the pack code understand the size of the response either in absolute terms or relative to the maximum allowed?
I don't think we can do that 100% of the time (like maybe if a server returns data with chunked response encoding we could see that the data is too large, but we don't necessarily know how large it would have been) so I'm fine with making this error vague for now and leaving the option to add more detail in the future if we think it's useful
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.
I could certainly put an estimate of the response size + the limit that was violated. Not sure if they bytes limit is that helpful but could be worth a shot
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 isn't a blocking comment, so if you have a plan to use this that doesn't depend on getting numbers back in the response I'm fine with that as long we there's no API structure objections from Patrick or Eric
(One thing I thought about re: API was whether this could be expressed as a StatusCodeError, but HTTP only seems to have an official status code for request to big, but not standard error code for response would be too big)
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.
I also do not want to confuse StatusCodeErrors and errors that are imposed by Coda. I think that could be potentially confusing to the consumer
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.
API and docs LGTM!
* | ||
* @hidden | ||
*/ | ||
export class ResponseSizeTooLargeError extends Error { |
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.
RequestTooLarge would also be good to add at some point, but doesn't need to block this.
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.
Will add as needed. Starting with this. Once this is going in a real pack can add the other error as well will be very similar
* add response too large error * bump version to pre-release
aef15fe
to
f7a15df
Compare