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

proposal: openapi-fetch new send function #1558

Open
rozbo opened this issue Feb 21, 2024 · 7 comments
Open

proposal: openapi-fetch new send function #1558

rozbo opened this issue Feb 21, 2024 · 7 comments
Labels
bug Something isn't working openapi-fetch Relevant to the openapi-fetch library

Comments

@rozbo
Copy link

rozbo commented Feb 21, 2024

background

In #1122 we discussed that resend request in middleware, it's hard to do that in current middleware design. But things like refresh token in real production, this is a very useful feature. So I want to openapi-fetch can do that.

investigation

for other request lib like axios, it alose provide a way to do this, Their methods are similar. Basically, when an error occurs or during the response phase, the callback is called with the requested config as a parameter, and then can call the send again.
for axios, there is a function request, and the requestConfig config.

plan

This is a mature experience that I think we can consider learning from.

we can do that for this step.

  1. add new coreSend function just to send request with middware, it accept the Stand fetch Request param and return the Stand fetch Response. it's the warpper of fetch.
  2. refactor coreFetch to invoke coreSend
  3. export to coreSend to user with send, this function like axios's request.
  4. invoke middware with request, but fetch's Reqeust can't be read again, we don't want to clone it since memory waste, so we need it's constructor new Request(input, options) url and options, this is exist coreFetch alreay, we put this 2 param to mergeOption object. '

after that, use can remake the Request object by url and requestOption from mergedOptions, and resend it by send. This is no BREAK CHANGE now.

the only BREAK CHANGE is optional, I moved schemaPath and params from MiddlewareRequest to mergedOptions, just because I don't want to pollute the request object, we should put it to own objects like mergedOptions. but for users, the BREAK CHANGE is more important, so this break change can be optional.

for reference only

#1556

@rozbo rozbo added bug Something isn't working openapi-fetch Relevant to the openapi-fetch library labels Feb 21, 2024
@rozbo
Copy link
Author

rozbo commented Feb 21, 2024

sry for choosed wrong label, help me pls!

@drwpow
Copy link
Owner

drwpow commented Feb 23, 2024

You’ve described how the internals should change, but I’d love to see this from the perspective of the end-user. What code do they write? What does the API look like? What could they not do before but now they can? (is there any way to accomplish it with the current API, or was it impossible)? I’m more interested in how writing middleware could be easier with this change, rather than describing what a PR would do codewise.

@rozbo
Copy link
Author

rozbo commented Feb 23, 2024

It mainly solves the issue here #1122 (comment) , user can use it to resend some request in middleware like this.

client.use({
          async onResponse(response, options) {
            if(response.status === 401) {
              // do some other request...
              // then resend it.
              return  await client.send(new Request(options.requestUrl,options.requestOptions), options);
            }
            return response;
          },
        });

@rozbo
Copy link
Author

rozbo commented Feb 23, 2024

Currently, a simple way to solve this problem is to use a custom fetch function.
But, to be honest, using a custom fetch function, with more code, user can achieve everything that middleware can do. In that case, what is the meaning of middleware?

@drwpow
Copy link
Owner

drwpow commented Mar 16, 2024

It mainly solves the issue here #1122 (comment) , user can use it to resend some request in middleware like this.

Ah I see. You’re right, I haven’t explicitly replied to that, but that is something I would definitely not want to add to the library—retries. I think retry mechanisms should always be handled at a layer above openapi-fetch, in its current design. Reason is, it inevitably bleeds into the caching and orchestration layer (e.g. React Query) which have retry mechanisms baked in. I do not want this library to take on caching, or orchestration, or retries, ever. I want this to remain a low-level typed fetch wrapper that can be used with anything on top of it, and therefore work in any stack and setup.

Perhaps in the future I’ll reverse the decision, but I feel this won’t happen in 0.x, and I haven’t started planning what 1.x would look like.

But, to be honest, using a custom fetch function, with more code, user can achieve everything that middleware can do. In that case, what is the meaning of middleware?

That’s true. Though it’s not always a design flaw to be able to do things in multiple ways! Custom fetch can own everything, but requires you to manage 100% of the code. Middleware is more modular and DRY, and also lets you use multiple third-party packages that all plays nicely with your code. There’s a reason why most libraries use this pattern.

@RobinClowers
Copy link

I agree that this library shouldn't have first class support for retries or caching! However, providing "around" style middleware e.g. (request: Request, next: Middleware) => Request gives end users total flexibility to build retries or whatever other feature they want on top of this library. Because data fetching is so foundational to many apps, I think that sort of extensibility is one of the differences between good and great libraries.

It's great that you have the custom fetch option, and maybe that's what people should reach for if they need that flexibility. However, it doesn't seem like much of a lift compared to the existing middleware implemenation.

@djMax
Copy link

djMax commented Apr 27, 2024

On a related note, the current middleware doesn't provide the option to catch fetch errors it seems? onResponse is not called when the fetch throws, which makes some sense, but makes a common use of middleware difficult and/or puts you back to the "just use a custom fetch function" option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working openapi-fetch Relevant to the openapi-fetch library
Projects
None yet
Development

No branches or pull requests

4 participants