Skip to content

Conversation

@Corepex
Copy link
Contributor

@Corepex Corepex commented Mar 22, 2024

Followup to: #46

Problem

It seems that the headers dont get passed to the request.

Fix

I created a fix for that but it should be considered as "quick and dirty".
IMHO there is a bigger problem with this part of the code generation. Also the names of the props are weird.

Copy link
Collaborator

@seriouslag seriouslag left a comment

Choose a reason for hiding this comment

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

How is this a bug? Does this solve an issue with the tests or fix the example?

Or should this change be added to the base request file?

@Corepex
Copy link
Contributor Author

Corepex commented Mar 22, 2024

@seriouslag - yeah this - or something smarter - should be added to the request file ...

It is a bug because "special headers" like Content-Type: application/merge-patch+json don't get passed from the generated service to the request.

In my case (for example) the PATCH method looks like that:
image

@seriouslag
Copy link
Collaborator

seriouslag commented Mar 22, 2024

I see your case. This library uses the package openapi-typescript-codegen for service generation. The default request file comes from that library. I am all for documenting your use case and fix in this package, but an issue/PR in that library may be better for the ecosystem. This PR changes the example code, not the code the end user will see. I can see how it would be helpful to have different request files in this library for examples of how to use this library.

@7nohe
Copy link
Owner

7nohe commented Mar 29, 2024

As @seriouslag said, the request file is not the responsibility of this library.
However, I think it makes a good example of how to customize headers, so I will accept this PR.

@7nohe 7nohe merged commit 4d97317 into 7nohe:main Mar 29, 2024
@Corepex Corepex deleted the fix_missing_headers branch March 29, 2024 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants