Skip to content

feat(axios): use named parameters #887

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

Closed
wants to merge 6 commits into from

Conversation

Marcel-G
Copy link
Contributor

@Marcel-G Marcel-G commented Jul 3, 2023

Status

READY

Description

Often times routes have many path parameters. As the number grows, type-safety is less effective as arguments (commonly string or number types) can easily get used in the wrong order.
Using named parameters helps this, as you get the added safety of needing to specify the appropriate name for the parameter.

This PR adds a useNamedParameters option for the axios client which creates path parameters as a single object.

Steps to Test or Reproduce

Outline the steps to test or reproduce the PR here.

> git pull --prune
> git checkout Marcel-G:feat/named-parameters
> yarn build
> cd /tests
> yarn generate
> vi generated/axios/named-parameters/endpoints.ts

Heres an example output:

/**
 * @summary Info for a specific pet
 */
export const showPetById = <TData = AxiosResponse<Pet>>(
  { petId, version = 1 }: ShowPetByIdPathParameters, options?: AxiosRequestConfig
): Promise<TData> => {
  return axios.get(
    `/v${version}/pets/${petId}`, options
  );
}
type ShowPetByIdPathParameters = {
  petId: string,
  version?: number,
}

@vercel
Copy link

vercel bot commented Jul 3, 2023

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

@anymaniax first needs to authorize it.

@Marcel-G Marcel-G marked this pull request as ready for review July 3, 2023 15:54
@anymaniax
Copy link
Collaborator

it's doesn't seems to work properly when there is no path params

@Marcel-G
Copy link
Contributor Author

@anymaniax this change seems to have some overlap with #396. Do you have a preference between the two implementations?

@anymaniax
Copy link
Collaborator

Hello @Marcel-G and thanks a lot. Seems great! Would you be interested to add a global config for that and do it for all client instead?

@Marcel-G
Copy link
Contributor Author

Hello @Marcel-G and thanks a lot. Seems great! Would you be interested to add a global config for that and do it for all client instead?

Gave it a shot, heres what I came up with #914

@anymaniax
Copy link
Collaborator

@Marcel-G do you think we can merge the other one directly?

@Marcel-G
Copy link
Contributor Author

@Marcel-G do you think we can merge the other one directly?

Actually this PR goes with a slightly different approach so the other PR is redundant if we go this way.

@anymaniax
Copy link
Collaborator

@Marcel-G for me other one make more sense to have a more global way of doing it. What do you think?

@anymaniax
Copy link
Collaborator

@Marcel-G can you also contact me on discord when you have the time?

@Marcel-G
Copy link
Contributor Author

Closing this for a more general approach #914

@Marcel-G Marcel-G closed this Aug 24, 2023
@Marcel-G Marcel-G deleted the feat/named-parameters branch August 24, 2023 14:33
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.

2 participants