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

Improve 204 support #24

Merged
merged 15 commits into from
Jul 8, 2024
Merged

Improve 204 support #24

merged 15 commits into from
Jul 8, 2024

Conversation

kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Jul 8, 2024

Changes

ToDo:

  1. Tests for GET with 204 response cases
  2. Tests for non-JSON get responses
  3. Tests for different variants of isEmptyResponseExpected/isNonJSONResponseExpected params
  4. Look into "unsafe any" linting error
  5. Extra documentation

Checklist

  • Apply one of following labels; major, minor, patch or skip-release
  • I've updated the documentation, or no changes were necessary
  • I've updated the tests, or no changes were necessary

@kibertoad kibertoad added the minor label Jul 8, 2024
@kibertoad kibertoad requested a review from CarlosGamero July 8, 2024 10:00
src/client.ts Outdated

if (bodyParseResult.error === 'EMPTY_RESPONSE') {
if (params.isEmptyResponseExpected) {
return response as unknown as Promise<ResponseBody>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ondrejsevcik @szymonchudy How would FE prefer to be communicated about special cases? Currently it would just return entire Response object in case something isn't considered a direct error, but is an unusual situation (e. g. non-JSON or expected as possible empty content came back). Is that good enough? We can return an Either with a defined set of possible result codes instead.

Copy link
Collaborator

@ondrejsevcik ondrejsevcik Jul 8, 2024

Choose a reason for hiding this comment

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

If the isEmptyResponseExpected is true, then this is not really an error case from the point of the caller. I would expect that it would return successful promise resolution with undefined/void value.

The code could check the response status, and if it is 204, then the parsing should be skipped IMO.

Copy link
Collaborator Author

@kibertoad kibertoad Jul 8, 2024

Choose a reason for hiding this comment

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

If the isEmptyResponseExpected is true, then this is not really an error case from the point of the caller. I would expect that it would return successful promise resolution with undefined/void value.

Fair point. We can use a more bespoke structure then Either then, with three possible fields: Left (error), Middle (non-error deviation from main path) Right (expected main path response). If Right is set, we proceed with the main path, if left is set, we handle an error, if Middle is set, we check the code returned and handle it in a custom way.

The code could check the response status, and if it is 204, then the parsing should be skipped IMO.

Aren't we already doing that in this PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@kibertoad maybe I'm looking at a wrong line, but I think we're not checking the status before parsing.

CleanShot 2024-07-08 at 13 33 37@2x

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ondrejsevcik check the tryToResolveJsonBody itself

@CarlosGamero We probably should have the check in the consistent place for both GET and non-GET scenarios, I think currently I did it explicitly for non-GET and implicitly (within tryToResolveJsonBody) for GET.

Copy link
Contributor

@CarlosGamero CarlosGamero Jul 8, 2024

Choose a reason for hiding this comment

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

@kibertoad I will change it to use the same approach on both places 🙇

Copy link
Contributor

Choose a reason for hiding this comment

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

Already changed

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@CarlosGamero CarlosGamero added major and removed minor labels Jul 8, 2024
@kibertoad kibertoad marked this pull request as ready for review July 8, 2024 15:32
@kibertoad kibertoad merged commit 2ea5768 into main Jul 8, 2024
5 checks passed
@kibertoad kibertoad deleted the feat/204-support branch July 8, 2024 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants