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

common: request throws if parseAs is json and the response body is empty #906

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

hunterachieng
Copy link
Contributor

@hunterachieng hunterachieng commented Jan 14, 2025

Summary

An error is thrown when we use parseAs=json if the response body is undefined when PUT or POST requests are made

Fixes #689

Details

In common, we mainly parse the response body, but since we might get an empty body with different requests, we are returning undefined for the body else parse as expected

AI Usage

Please disclose how you've used AI in this work (it's cool, we just want to know!):

  • Code generation (copilot but not intellisense)
  • Learning or fact checking
  • Strategy / design
  • Optimisation / refactoring
  • Translation / spellchecking / doc gen
  • Other
  • I have not used AI

You can read more details in our Responsible AI Policy

Review Checklist

Before merging, the reviewer should check the following items:

  • Does the PR do what it claims to do?
  • If this is a new adaptor, added the adaptor on marketing website ?
  • Are there any unit tests?
  • Is there a changeset associated with this PR? Should there be? Note that
    dev only changes don't need a changeset.
  • Have you ticked a box under AI Usage?

@josephjclark
Copy link
Collaborator

Hi @hunterachieng, can you update the PR description please? You need to:

  • Include a brief description of the fix
  • update the AI disclaimer
  • remove any template sections that aren't relevant to this PR

.changeset/gold-kings-grow.md Outdated Show resolved Hide resolved
.changeset/gold-kings-grow.md Outdated Show resolved Hide resolved
packages/common/src/util/http.js Outdated Show resolved Hide resolved
packages/common/src/util/http.js Outdated Show resolved Hide resolved
packages/commcare/src/Utils.js Outdated Show resolved Hide resolved
packages/common/test/http.test.js Outdated Show resolved Hide resolved
Signed-off-by: Hunter Achieng <[email protected]>
Signed-off-by: Hunter Achieng <[email protected]>
Copy link
Collaborator

@josephjclark josephjclark left a comment

Choose a reason for hiding this comment

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

Almost there - just a couple of comments left to address. It's worth getting this right: change to common can affect every other adaptor, so getting it right now will save us problems in other adaptors in the future (if we'd done this right in the first place, you wouldn't be having to do this at all!)

.changeset/smart-eggs-smile.md Outdated Show resolved Hide resolved
packages/common/src/util/http.js Outdated Show resolved Hide resolved
packages/common/test/util/http.test.js Show resolved Hide resolved
Signed-off-by: Hunter Achieng <[email protected]>
@josephjclark
Copy link
Collaborator

Hi @mtuchi - can you run pnpm changeset version on this PR, push the diffs, and merge? Hunter is blocked by it.

Remember we don't have to push tags any more :) Just push the version bumps and merge to release. Don't worry about the warnings when you version.

Thanks! I'll be online around 10am GMT if there are any problems. You'll be online way before me and can unblock Hunter quicker 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

common: request throws if parseAs is json and the response body is empty
2 participants