Skip to content

bugfix: Use merged request params to get request content-type #1227

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

am-burban
Copy link

Hello 👋

I just noticed that to determine the content-type of a request, only the type parameter from the request method itself is used. A type parameter provided in the baseApiParams when instantiating the HttpClient is ignored. Imho it's a good idea to pull the type from the merged request params, which also contain the global defaults.

Best,
Ben

Copy link

changeset-bot bot commented May 15, 2025

⚠️ No Changeset found

Latest commit: bf90e82

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@smorimoto smorimoto requested a review from Copilot May 15, 2025 14:06
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR ensures the Content-Type header is derived from the merged requestParams (including global defaults) instead of a local type variable.

  • Updated test snapshots to expect requestParams.type instead of type.
  • Adjusted the fetch HTTP client template to use requestParams.type.

Reviewed Changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated no comments.

File Description
tests/spec/**/__snapshots__/*.snap Updated Content-Type header logic in snapshots to use requestParams.type
tests/__snapshots__/*.snap Updated Content-Type header logic in snapshots to use requestParams.type
templates/base/http-clients/fetch-http-client.ejs Use requestParams.type for Content-Type header in fetch client generator
Comments suppressed due to low confidence (2)

templates/base/http-clients/js-http-client.ejs:183

  • The axios HTTP client template still references the local type variable instead of requestParams.type, which will ignore defaults from baseApiParams. Update this to use requestParams.type to match the fetch client.
...(type && type !== ContentType.FormData ? { "Content-Type": type } : {})

templates/base/http-clients/fetch-http-client.ejs:186

  • [nitpick] Consider extracting the Content-Type header merge logic into a shared helper or template partial to avoid duplicating this snippet across both fetch and axios client templates.
...(requestParams.type && requestParams.type !== ContentType.FormData ? { "Content-Type": requestParams.type } : {})

@smorimoto smorimoto added the bug Something isn't working label Jun 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants