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

(fix) add response headers in fetch client #1695

Closed
wants to merge 2 commits into from

Conversation

melloware
Copy link
Collaborator

Fix #1694 add response headers in fetch client

@melloware melloware requested a review from soartec-lab November 8, 2024 13:58
@melloware melloware added the fetch Fetch client related issue label Nov 8, 2024
@melloware melloware added this to the 7.2.1 milestone Nov 8, 2024
@melloware melloware force-pushed the 1694 branch 2 times, most recently from 1d4e98c to ac9da7c Compare November 8, 2024 14:19
@soartec-lab
Copy link
Member

Hi @melloware, Please give me some time to think about this response.

@soartec-lab soartec-lab self-assigned this Nov 8, 2024
@melloware melloware marked this pull request as draft November 8, 2024 14:32
@melloware
Copy link
Collaborator Author

Yep converting to draft was just seeing if that is what the OP meant!

@nimo23
Copy link

nimo23 commented Nov 8, 2024

@melloware Thanks, the PR looks good to me. I think Orval doesn't need to check to add the headers to the response only when custom headers are detected in openapi.yaml. Headers should be included by default.

Copy link
Member

@soartec-lab soartec-lab left a comment

Choose a reason for hiding this comment

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

Good update. I have added comments where additional updates are needed.

packages/fetch/src/index.ts Outdated Show resolved Hide resolved
packages/fetch/src/index.ts Outdated Show resolved Hide resolved
@melloware
Copy link
Collaborator Author

@soartec-lab you are more familiar with this code did you want to submit a PR and I can close this one?

@melloware
Copy link
Collaborator Author

OK changes made

@melloware melloware force-pushed the 1694 branch 2 times, most recently from ee8e99d to 4481143 Compare November 9, 2024 12:06
@melloware melloware marked this pull request as ready for review November 9, 2024 13:44
@melloware melloware requested a review from soartec-lab November 9, 2024 13:44
@soartec-lab
Copy link
Member

@melloware

Thanks!
can you regenerate the fetch client and update the custom-fetch implement in sample apps ?

@melloware
Copy link
Collaborator Author

@soartec-lab not sure what you mean by regenerate but i updated orval.config.js and custom-fetch.js in samples directory.

@soartec-lab
Copy link
Member

@melloware

Sorry for confusing you. To be more precise, I would like you to regenerate the sample app using the following command.

cd samples
yarn generate-api --force

Your change adds headers to returnType, so the returnType of the sample app generated by orval should also be updated.

@melloware
Copy link
Collaborator Author

Weird when I run that command in samples I get..

 Tasks:    0 successful, 21 total
Cached:    0 cached, 21 total
  Time:    1.957s
Failed:    react-query-basic#generate-api, react-query-form-data#generate-api, react-query-form-url-encoded#generate-api

 ERROR  run failed: command  exited (1)

@melloware
Copy link
Collaborator Author

Would you like to just merge this PR and then you can take a look?

@soartec-lab
Copy link
Member

@melloware
Can I run it and put commits on this branch ?

@melloware
Copy link
Collaborator Author

Oh sure thing my branch should be editable for you!

@soartec-lab
Copy link
Member

soartec-lab commented Nov 10, 2024

@melloware
Can you try cherry-pick this ?

13533d5

Or if you like, you can switch to this PR.

#1699

@melloware melloware closed this Nov 10, 2024
@melloware melloware deleted the 1694 branch November 10, 2024 18:57
@melloware melloware removed this from the 7.2.1 milestone Nov 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fetch Fetch client related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fetch: add headers to response
3 participants