openapi-typescript icon indicating copy to clipboard operation
openapi-typescript copied to clipboard

proposal: openapi-fetch new send function

Open ohroy opened this issue 1 year ago • 7 comments

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

ohroy avatar Feb 21 '24 07:02 ohroy

sry for choosed wrong label, help me pls!

ohroy avatar Feb 21 '24 07:02 ohroy

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.

drwpow avatar Feb 23 '24 00:02 drwpow

It mainly solves the issue here https://github.com/drwpow/openapi-typescript/issues/1122#issuecomment-1952298096 , 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;
          },
        });

ohroy avatar Feb 23 '24 08:02 ohroy

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?

ohroy avatar Feb 23 '24 08:02 ohroy

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.

drwpow avatar Mar 16 '24 12:03 drwpow

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.

RobinClowers avatar Apr 26 '24 22:04 RobinClowers

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.

djMax avatar Apr 27 '24 01:04 djMax

This issue is stale because it has been open for 90 days with no activity. If there is no activity in the next 7 days, the issue will be closed.

github-actions[bot] avatar Aug 06 '24 12:08 github-actions[bot]

This issue was closed because it has been inactive for 7 days since being marked as stale. Please open a new issue if you believe you are encountering a related problem.

github-actions[bot] avatar Aug 14 '24 02:08 github-actions[bot]