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

(Auto) Abortable API #1126

Closed
drwpow opened this issue Mar 17, 2023 · 6 comments
Closed

(Auto) Abortable API #1126

drwpow opened this issue Mar 17, 2023 · 6 comments
Labels
enhancement New feature or request openapi-fetch Relevant to the openapi-fetch library PRs welcome PRs are welcome to solve this issue! question Further information is requested

Comments

@drwpow
Copy link
Contributor

drwpow commented Mar 17, 2023

Another idea I’ve been thinking about is either making requests auto-abortable, or providing an easier API than the standard AbortController API when it comes to resubmitting requests. Some ideas:

Option A: implicit params match

Requests that hit the same endpoint with the same options will be automatically aborted. This is “magic” and also will increase the project weight by a significant amount by adding in the equality evaluation (which may also be buggy).

const fetch1 = get('/project/{project_id}', {params:{path:{project_id: 'abc'}}});
const fetch2 = get('/project/{project_id}', {params:{path:{project_id: 'xyz'}}}); 
const fetch3 = get('/project/{project_id}', {params:{path:{project_id: 'xyz'}}});

await fetch1; // returned 'abc' as normal
await fetch2; // silently aborted; `fetch3` returned because it passed identical options
await fetch3; // returned 'xyz' as normal

Option B: explicit ID

This is less magic, will have almost no impact on library weight, and won’t be buggy.

const fetch1 = get('/project/{project_id}', {params:{path:{project_id: 'abc'}}});
const fetch2 = get('/project/{project_id}', {params:{path:{project_id: 'abc'}}, operationId: 'project_abc'}); 
const fetch3 = get('/project/{project_id}', {params:{path:{project_id: 'abc'}, operationId: 'project_abc'}});

await fetch1; // returned as normal as there’s no `operationId`
await fetch2; // silently aborted; `fetch3` returned
await fetch3; // returned as normal

Note that in both instances, I think it’s only correct that the silently aborted call be replaced with the call that overrode it. Because if it simply erred, then it would cause the implementor to have to deal with an entirely new layer of typechecking (i.e. rather than simply handling data or error, they’d now have to always handle data, error, or aborted). There’d be no easy way for this library to match the abort error shape to the API shape, since the API shapes happen statically whereas the abort error happens at runtime (which has no knowledge of any types).

The “don’t add another typechecking layer” restriction is one I feel strongly about, since this library is meant to reduce complexity and boilerplate and not add to it.

@drwpow drwpow added enhancement New feature or request PRs welcome PRs are welcome to solve this issue! question Further information is requested labels Mar 17, 2023
@drwpow drwpow transferred this issue from drwpow/openapi-fetch May 22, 2023
@drwpow drwpow added the openapi-fetch Relevant to the openapi-fetch library label May 22, 2023
@HugeLetters
Copy link
Contributor

HugeLetters commented Oct 7, 2023

I would like to weigh in on this as a regular library consumer - I think it's best to leave such functionality to libraries like tanstack-query. They already support query auto-cancellation with signals and deduplication.

If you provide that functionality - people will still reach out to solutions like tanstack because it provides a lot of other useful functionality on top: caching, cache invalidation, pre-fetching, refetch on mount, focus etc, retries and many more. So either openapi-fetch would have to provide those too(and framework specific solutions as well) or people will use both libraries in tandem but now a lot of double work is being done. Tanstack uses first request as a base for deduplication - your solution seems to suggest you will use the last one as a base. Now there could be hard to debug problems due to this because it's hard to understand what even cancelled the request.

But once again that's just my pov of what I expect a library to do.

Perhaps another route is to go tRPC way and to publish some compatibility layer which will make it easier to use openapi-fetch and tanstack-query together. This also might be much easier to implement since a lot of functionality is already done for you and all you need to do is just implement a way for your two libraries to communicate.

@drwpow
Copy link
Contributor Author

drwpow commented Oct 19, 2023

That’s a great point. And I have been using React Query (Tanstack) in a production app and I agree—it provides caching, stale request refetching, and other utilities that are separate concerns from this library. I’ve been using openapi-fetch with it pretty successfully.

I think that’s a good enough reason to keep this library minimal and not concern itself with higher-order things like this (there are some minor API QoL improvements over the raw fetch() API, but nothing that fundamentally changes how it works or introduces foreign concepts).

Open to revisiting this in the future, but I think it’s not a priority for now.

@drwpow drwpow closed this as completed Oct 19, 2023
@HugeLetters
Copy link
Contributor

@drwpow I think making a small compat layer would be nice - I don't think it would need to provide any functionality beyond generating a key automatically with an api call based on verb, path, params & body. And also being able to get a key by providing a verb, path, input your self or something like that - for query invalidations and such. Everything else can be solved in user-land and so you don't even need framework-specific adapters. We just provide a user with a queryFn and a queryKey and they plug those in themselves.

I did that already for our project but it was a bit hacky due to how TS doesn't handle generics within generics well and I didn't wanna patch the library - but it works more or less but it could be more stable if it were done on a library level.
So if you're interested I could try working on that.

@drwpow
Copy link
Contributor Author

drwpow commented Oct 19, 2023

I think for now, I’d be interested in maybe expanding the React Query example before trying to turn it into an actual maintained library.

For us, we started off with a lot of duplicated code that seemed like it could be deduped. But very quickly, each query/mutation evolved to handle unique concerns based on the endpoint (for example, some queries would invalidate others’ keys for refetching; other mutations would fire background API calls that partially updated data caches, etc.). And there’s no clear/sane way we could automate those unique concerns. So in at least one real-world example, there’s not a compat layer we could write that wouldn’t be overridden in about every case with only a couple weeks of usage. With all the flexibility React Query provides I’m not confident a compat layer could exist that would help most people (at least not until I could see it for myself and kick the tires).

But again, showing an example of a good pattern is always helpful! And could possibly yield ideas to turn it into a library one day.

@HugeLetters
Copy link
Contributor

HugeLetters commented Oct 20, 2023

@drwpow

My idea of a compat layer(and what I did in our project) was basically this

declare function createQueryData(verb, path, input): QueryData;
type QueryData = { 
  // this input is merged with the one from createQueryData
  runQuery: (input) => void, 
  queryKey: [[path, verb?], input?] 
}; 
declare function getQueryKey(queryData: QueryData, type: "precise" | "route & verb" | "route & all verbs"): QueryData.queryKey

And then you just create your queryData objects and plug them into tanstack query functions however you like - you have a queryKey and you have a runQuery which you invoke inside of queryFn so may pass in signal. For invalidation you use getQueryKey and for example with such key structure you mare invalidate specific cache entry or an entire route, with all verbs or only a specific one.

Here's a gist with a more concrete example of my implementation - it's quite hacky but I believe a more "pretty" solution could be achieved if this was done more on a library level.

So it's the responsibility of a user to pass in generated key explicitly and to use these functions at all, it's not done implicitly like in trpc-query, but on the other hand it's a flexible solution which doesn't require to create a framework specific solution each time, you can use it with any framework.

@PierBover
Copy link

Personally I would like to see abort and other features somehow integrated with this library. Maybe not in the core but as am official plugin.

Plenty of apps not using React Query or even React.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request openapi-fetch Relevant to the openapi-fetch library PRs welcome PRs are welcome to solve this issue! question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants