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(core): capitalize http verb patch #905

Closed
wants to merge 8 commits into from
Closed

Conversation

hkfi
Copy link

@hkfi hkfi commented Aug 2, 2023

Status

READY

Description

Changed the HTTP verb 'patch' to 'PATCH'.
PATCH is the only HTTP method that is case sensitive. It needs to be capitalized to work.

Related PRs

List related PRs against other branches:

None

Todos

  • Tests
  • Documentation
  • Changelog Entry (unreleased)

@anymaniax
Copy link
Collaborator

I never had that issue. In which case do have that? An what http client do you use and if it’s a dependency which version?

@hkfi
Copy link
Author

hkfi commented Aug 15, 2023

When using JavaScript's fetch.

Another person described the problem here in the issues as well:
#888

@vercel
Copy link

vercel bot commented Aug 15, 2023

Someone is attempting to deploy a commit to a Personal Account owned by @anymaniax on Vercel.

@anymaniax first needs to authorize it.

@anymaniax
Copy link
Collaborator

@hkfi the problem it's that currently done like that It would be a breaking change and you can uppercase all method in your custom instance. Would be better to create a fetch client no?

@hkfi
Copy link
Author

hkfi commented Aug 17, 2023

@anymaniax
I suppose it would be a breaking change in the sense that the generated string would be different, so it would "break" for users who are depending on a 'patch' instead of 'PATCH'. However, 'PATCH' will always work and 'patch' may not work, depending on how the custom client is set up.

I think the generated strings should be aligned with the HTTP verb standards anyway and should by default all be uppercase. If for whatever reason, generating 'PATCH' instead of a 'patch' results in a break in their PATCH requests, that should be a mistake in how the user sets up their client or sending requests rather than a mistake in the code generated by Orval.

Right now, the code that is generated is wrong and unusable without customizing the client to account for this, which shouldn't be the expectation. The generated code should work out of the box.

@anymaniax
Copy link
Collaborator

Then would be better to pass everything to uppercase no?

@Jacob-Roberts
Copy link

The only verb where uppercase is required is PATCH, so it doesn't need to be every verb updated as part of this PR, but yes it could be every verb

melloware
melloware previously approved these changes Oct 20, 2023
@melloware
Copy link
Collaborator

It should be every verb. They all should be uppercase

@melloware
Copy link
Collaborator

Looks like this is failing the ZOD compilation because PathItemObject from openapi-ts

https://github.com/drwpow/openapi-typescript/blob/43b1b69bc26ee333d0fed1f4aa702496c485dc01/packages/openapi-typescript/src/types.ts#L166-L187

@melloware
Copy link
Collaborator

Superseded by PR: #1043

@melloware melloware closed this Nov 13, 2023
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.

4 participants