这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@franworks
Copy link

@franworks franworks commented Aug 30, 2025

resolves: #2553

When the options.serializedBody property was introduced, it replaced the body sent in the Request object causing any options.body not requiring a bodySerializer (ie plain/text) to be omitted from the request.

This PR fixes this behavior, ensuring that when an options.body is defined it is sent with the Request 'as-is', or 'serialized' if an options.bodySerializer is defined.

@bolt-new-by-stackblitz
Copy link

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@changeset-bot
Copy link

changeset-bot bot commented Aug 30, 2025

🦋 Changeset detected

Latest commit: f653716

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

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

@vercel
Copy link

vercel bot commented Aug 30, 2025

Someone is attempting to deploy a commit to the Hey API Team on Vercel.

A member of the Team first needs to authorize it.

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. bug 🔥 Something isn't working client Client package related labels Aug 30, 2025
@franworks
Copy link
Author

@mrlubos assistance is requested to address next steps in resolving pipeline errors.

Copy link
Member

@mrlubos mrlubos left a comment

Choose a reason for hiding this comment

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

Please update the logic in all clients using serializedBody and then update snapshots with pnpm --filter @test/openapi-ts test:update

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Aug 31, 2025
@pkg-pr-new
Copy link

pkg-pr-new bot commented Aug 31, 2025

Open in StackBlitz

npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/codegen-core@2564
npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/nuxt@2564
npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/openapi-ts@2564
npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/vite-plugin@2564

commit: f653716

@codecov
Copy link

codecov bot commented Aug 31, 2025

Codecov Report

❌ Patch coverage is 96.15385% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 24.05%. Comparing base (62cff55) to head (f653716).
⚠️ Report is 16 commits behind head on main.

Files with missing lines Patch % Lines
...c/plugins/@hey-api/client-angular/bundle/client.ts 94.11% 1 Missing ⚠️
.../src/plugins/@hey-api/client-next/bundle/client.ts 94.73% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2564      +/-   ##
==========================================
+ Coverage   23.59%   24.05%   +0.46%     
==========================================
  Files         363      363              
  Lines       36545    36585      +40     
  Branches     1562     1628      +66     
==========================================
+ Hits         8622     8802     +180     
+ Misses      27910    27770     -140     
  Partials       13       13              
Flag Coverage Δ
unittests 24.05% <96.15%> (+0.46%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@franworks
Copy link
Author

@mrlubos the next client was the only client impacted by the serializeBody change - that has issue has now been resolved as part of this PR.

Tests have been added, snapshots updated.

@mrlubos
Copy link
Member

mrlubos commented Aug 31, 2025

@mrlubos the next client was the only client impacted by the serializeBody change - that has issue has now been resolved as part of this PR.

Tests have been added, snapshots updated.

Isn't this line going to have the same issue?

@franworks
Copy link
Author

@mrlubos I reviewed the changes tracked in the issue that triggered the bug which were the client-fetch and client-next clients.

I did not go through and review all of the clients. However, I can do that, starting with your reference to the angular client.

@mrlubos
Copy link
Member

mrlubos commented Aug 31, 2025

@mrlubos I reviewed the changes tracked in the issue that triggered the bug which were the client-fetch and client-next clients.

I did not go through and review all of the clients. However, I can do that, starting with your reference to the angular client.

Yes please. Aside from making sure this bug isn't present in any of the clients, I think there are 2 issues with this change.

  1. if (opts.serializedBody) { requires the serialized body to be truthy, which would not be true for empty strings, 0, etc. I think it should be checking for the presence of serialized body instead fix(client-fetch): ensures plain/text body is sent with request #2564 (comment)
  2. if (opts.body === undefined || opts.body === '') { relies on the raw body for removing the Content-Type header. This is incorrect, it should work based on the final body we'll be sending. Currently it makes it possible to have a body that serializes to empty string and we'd fail to remove the Content-Type header.

@franworks
Copy link
Author

@mrlubos the angular client would not have this issue because serializedBody is initialized with options.body

@franworks
Copy link
Author

@mrlubos - looking at the angular client, would be acceptable to initialize the serializedBody with the options.body?

@mrlubos
Copy link
Member

mrlubos commented Aug 31, 2025

That's either a mistake we want to fix or a valid approach we should use across all clients. I don't know what's the better approach, and I don't feel particularly strongly about either option. The only thing I feel strongly about is having it be consistent so one can easily reason through these features

@franworks
Copy link
Author

Agreed. I'm writing tests for the different scenarios. Will post back when I have it sorted out.

@franworks
Copy link
Author

@mrlubos I pushed just the fetch client changes. If these changes make sense, I will update the other clients.
I do not think serializedBody should be initialized with opts.body - it would be unexpected if no bodySerializer is defined.

@franworks franworks marked this pull request as draft September 1, 2025 14:54
@franworks
Copy link
Author

franworks commented Sep 1, 2025

@mrlubos A body defined as 0 or false with defined bodySerializer will not make it into the expected block

Should this check be updated as part of this PR?

@mrlubos
Copy link
Member

mrlubos commented Sep 2, 2025

@mrlubos A body defined as 0 or false with defined bodySerializer will not make it into the expected block

Should this check be updated as part of this PR?

Yes please

@mrlubos
Copy link
Member

mrlubos commented Sep 2, 2025

I do not think serializedBody should be initialized with opts.body - it would be unexpected if no bodySerializer is defined.

I agree

@franworks
Copy link
Author

Changing how the angular client works would be a breaking change, and I don't think that should be done as part of this PR.
I have the changes, I can put them on a branch and PR them separately.

@mrlubos
Copy link
Member

mrlubos commented Sep 2, 2025

Changing how the angular client works would be a breaking change, and I don't think that should be done as part of this PR. I have the changes, I can put them on a branch and PR them separately.

Sounds good! What would be the breaking change?

@franworks
Copy link
Author

Changing how the angular client works would be a breaking change, and I don't think that should be done as part of this PR. I have the changes, I can put them on a branch and PR them separately.

Sounds good! What would be the breaking change?

serializedBody will go from being initialized with options.body to being undefined, so in the case of unserialized data - it would break that functionality for anyone leveraging that value in the interceptor.

@franworks franworks marked this pull request as ready for review September 3, 2025 16:50
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Sep 3, 2025
@mrlubos
Copy link
Member

mrlubos commented Sep 4, 2025

Changing how the angular client works would be a breaking change, and I don't think that should be done as part of this PR. I have the changes, I can put them on a branch and PR them separately.

Sounds good! What would be the breaking change?

serializedBody will go from being initialized with options.body to being undefined, so in the case of unserialized data - it would break that functionality for anyone leveraging that value in the interceptor.

I think that's fine, what do you think? Isn't that how it works for the Fetch client too?

@franworks
Copy link
Author

franworks commented Sep 4, 2025

Changing how the angular client works would be a breaking change, and I don't think that should be done as part of this PR. I have the changes, I can put them on a branch and PR them separately.

Sounds good! What would be the breaking change?

serializedBody will go from being initialized with options.body to being undefined, so in the case of unserialized data - it would break that functionality for anyone leveraging that value in the interceptor.

I think that's fine, what do you think? Isn't that how it works for the Fetch client too?

The fetch client does work like that. I felt that a breaking change warranted it is own issue with a description of what changed and why.

However, if this is not a concern, I will push the change.

…s sent with request

BREAKING CHANGE: options.serializedBody is undefined by default, and will only be assigned a value when options.bodySerializer is defined

refs: hey-api#2553
@mrlubos
Copy link
Member

mrlubos commented Sep 4, 2025

Yeah I'm not overly concerned with it as both the Angular client and serialized body are relatively new changes. We can document it in release notes, the pull request looks ready now I think. I'll review it one last time and merge later, thank you for the big contribution!

@franworks
Copy link
Author

Yeah I'm not overly concerned with it as both the Angular client and serialized body are relatively new changes. We can document it in release notes, the pull request looks ready now I think. I'll review it one last time and merge later, thank you for the big contribution!

Thank you!

Copy link
Member

@mrlubos mrlubos left a comment

Choose a reason for hiding this comment

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

This looks good! I think we need to improve the Axios client as well, but happy to merge it

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Sep 4, 2025
@franworks
Copy link
Author

Ok, I can help with that. I'll create an issue and start looking into it later on this evening.

@mrlubos mrlubos merged commit 01b8086 into hey-api:main Sep 5, 2025
12 of 13 checks passed
@hey-api hey-api bot mentioned this pull request Sep 4, 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 client Client package related lgtm This PR has been approved by a maintainer size:XL This PR changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fetch client request body is undefined when content-type is text/plain

2 participants