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

feat: add new client fetch #1353

Merged

Conversation

soartec-lab
Copy link
Member

@soartec-lab soartec-lab commented May 3, 2024

Status

READY

Description

Follow up for #1387

I added a new fetchclient.
Becouse, since the fetch API is mature, it has the advantage of reducing the bundle size of the application compared to using Axios, and I feel that this is sufficient in some cases.
Furthermore, we think it can be used to select an HTTP client with tanstack query or swr, or when calling an API from a server-side framework.

In this PR, i implemented a client using the minimal fetch API. For example, customization by specifying mutator is not yet supported. However, I still think it is practical enough and would like to submmit it on first. I'll also submit a sample application in a subsequent PR.

Ref: https://developer.mozilla.org/en-US/docs/Web/API/fetch

Related PRs

none

Todos

  • Tests
  • Documentation
  • Changelog Entry (unreleased)

Steps to Test or Reproduce

I added a test case, so it should pass.

@soartec-lab soartec-lab added the enhancement New feature or request label May 3, 2024
@anymaniax
Copy link
Collaborator

the params are passed to the body. Is it intended?

@soartec-lab
Copy link
Member Author

thanks. I'll check later 👍

@melloware
Copy link
Collaborator

Also code conflict might want to merge update.

@anymaniax
Copy link
Collaborator

@soartec-lab it's what I use in my project if you need https://gist.github.com/anymaniax/44c1331a5643081a82da070e45f405f0

@soartec-lab
Copy link
Member Author

soartec-lab commented May 13, 2024

@anymaniax
wonderful. Thank you for sharing 🙌
I will use this as a reference when adding a custom instance in a future PR.

the params are passed to the body. Is it intended?

This is by design. Referring to this, I interpret that the request parameters are to be specified in the body.

https://developer.mozilla.org/en-US/docs/Web/API/fetch

@soartec-lab soartec-lab force-pushed the feat/add-new-client-fetch branch from e370e45 to f546fc4 Compare May 13, 2024 13:20
@anymaniax
Copy link
Collaborator

@soartec-lab If you pass the query params into the body it wont be a query params anymore or I am missing something?

@soartec-lab
Copy link
Member Author

@anymaniax
I see, the sample you shared with me adds query parameters to the URL. I will refer to it and consider it again.

@soartec-lab soartec-lab marked this pull request as draft May 14, 2024 10:45
@soartec-lab soartec-lab force-pushed the feat/add-new-client-fetch branch from f546fc4 to f8e5c55 Compare May 19, 2024 00:38
@soartec-lab soartec-lab self-assigned this May 19, 2024
Comment on lines 68 to 58
const normalizedParams = new URLSearchParams();

Object.entries(params || {}).forEach(([key, value]) => {
if (value !== null && value !== undefined) {
normalizedParams.append(key, value.toString());
}
});`
Copy link
Member Author

Choose a reason for hiding this comment

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

normalize it by excluding undefined and null, because query params join to fetch url.

@soartec-lab
Copy link
Member Author

Hi, @anymaniax

I took your advice into consideration and changed the URL used by fetch to be obtained from a get URL function. This function combines path parameters and query parameters. And they only use the request body for body parameters.

Cloud you review again ?

@soartec-lab soartec-lab marked this pull request as ready for review May 19, 2024 03:12
@soartec-lab soartec-lab requested a review from anymaniax May 19, 2024 03:12
@soartec-lab soartec-lab force-pushed the feat/add-new-client-fetch branch from c2f96ac to 47bfd13 Compare May 19, 2024 23:55
@melloware melloware linked an issue May 24, 2024 that may be closed by this pull request
7 tasks
@melloware melloware added this to the 6.30.0 milestone May 31, 2024
@soartec-lab soartec-lab force-pushed the feat/add-new-client-fetch branch from cc654b3 to ec07c15 Compare June 5, 2024 00:12
@soartec-lab soartec-lab requested a review from anymaniax June 5, 2024 00:16
@soartec-lab
Copy link
Member Author

@anymaniax
Thank you for the review. I have fixed all responses and commented.

I wanna add the fetch client incrementally and improve it little by little. First of all, I'm adding a minimal working package, so additional behaviors can be added in subsequent PRs. Detailed plans are also managed in this issue.

#1387

@soartec-lab
Copy link
Member Author

@melloware

Even if i merge this, I don't wanna close #1387, so I deleted fix #1387 in description, is there any problem?

@melloware
Copy link
Collaborator

No problem! We can always reopen if it accidentally closes. I always prefer to have PR linked to ticket so we can reference later.

@soartec-lab
Copy link
Member Author

Thanks !

@soartec-lab soartec-lab merged commit d1cf87f into orval-labs:master Jun 6, 2024
2 checks passed
@melloware melloware mentioned this pull request Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support fetch client
3 participants