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

cleanup tiny bit #211

Open
cyberhck opened this issue Oct 12, 2020 · 1 comment
Open

cleanup tiny bit #211

cyberhck opened this issue Oct 12, 2020 · 1 comment
Milestone

Comments

@cyberhck
Copy link
Collaborator

What I like about tinka is that it's very close to standard fetch, we are able to work with actual fetch (if we don't add any fancy middleware), and it's dependency free.

When I was working with the Response, I was surprised that it wasn't aligned with standard specs. Looking at FetchMiddleware, I see a few issues there.

First, it's not extending from standard Response (that was probably because Response wasn't available back then), now that it is, IFetchResponse should probably extend from Response and overwrite only .json() method since it's generic.

It also contains a cache key which I don't think belongs there,

I propose to fix this up, also looking at CacheMiddleware,

export interface ICacheMiddlewareStore {
setItem(key: string, value: string): void;
getItem(key: string): string | undefined;
removeItem(key: string): void;
}
there's no way anyone can implement this interface in a decent manner, we should work with promises, this cache middleware looks like it's designed by only thinking about localstore in mind.

Let's deprecate CacheMiddleware, update the fetch response interface (maybe also request), once this deprecate happens, then we can drop support for Cache for now (no one is using it from what I know), and let users implement their cache on how they want, Cache is such a vast topic that it even makes sense to create a separate package with a lot of Drivers (which implements ICache interface)

cc @paibamboo

@cyberhck
Copy link
Collaborator Author

also the difficult part of CacheMiddleware is that it pollutes the original object, which means when you request:

const sdk = new Sdk();
sdk.user.get(1, {cache: {ttl: 60}});

What I'd prefer what's mock middleware is doing, adding a middleware and attaching handlers to that middleware, this means user after setting up cache once, never has to think about it.

@cyberhck cyberhck mentioned this issue Oct 21, 2020
@cyberhck cyberhck added this to the V2 milestone Nov 11, 2020
@cyberhck cyberhck pinned this issue Nov 17, 2020
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

No branches or pull requests

1 participant