-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Possible to support object keys? #166
Comments
For simple cases, you could do something along the lines of what I did for Axios here: https://github.com/zeit/swr/tree/master/examples/axios It basically just wraps |
If you need a typescript example: https://github.com/zeit/swr/tree/master/examples/axios-typescript Although your case might be a bit simpler since that example also handles the |
@davecoates I like the idea 👍
However it's okay to wrap it with an array automatically internally: useSwr(object)
// equivalent to
useSwr([object]) Thanks @Svish for the |
If you wrap it in an array like that, just make sure it's actually the exact same object each time... i.e. that |
@Svish Yeah. The ideal use case of object keys is not for "params". The ideal case should be: const { data: user } = useSWR('/api/user', fetch)
const { data: userPhotos } = useSWR(['/api/photos', user], fetchUserPhoto) Because SWR deep compares & caches the data ( |
@quietshu Looks like you'd need a custom fetcher for every new data dependency type or something then? 🤔 const { data: user } = useSWR(['/api/user', userId], fetchUser)
const { data: userPhotos } = useSWR(['/api/photos', user], fetchUserPhoto)
const { data: post } = useSWR(['/api/post', postId], fetchPost)
const { data: comments } = useSWR(['/api/comments', post], fetchPostComments)
... |
@Svish If you want to use objects as the arguments — yes. But you can hide them inside custom hooks: const { data: user } = useUser(userId)
const { data: userPhotos } = useUserPhoto(user)
const { data: post } = usePost(postId)
const { data: comments } = usePostComments(post)
... But under the hood, it will always be a URL + headers in the HTTP request. So no matter which pattern you want to use inside your components, I'd always suggest to transform and pass it as a string key (w/ options) to function useUserPhoto(user, fetcherOpts = null) {
return useSWR(user ? ['/api/photos?uid=' + user.id, fetcherOpts] : null, fetcher)
} In that case you don't need any custom fetcher. |
@quietshu Don't think I like how that looks, hehe. Think I'll stick with my Anyways, hope @davecoates found the answer he needed! And I do agree it would be super handy if |
Yeah @Svish it's good to use what works the best in different cases 👍 What I was sharing is, this pattern makes great sense from the point of view of the app structure too. In complex web applications, you definitely don't want to put the APIs and fetch handlers inside every single component that uses the data. Instead you can have a And that works really well for us in the past 6 months. I'll share more details in the future with a concrete example. |
Yeah, that's pretty much what we do now, except we have an // src/api/foobar/index.ts
import { ApiRequest, ApiRequestDetails } from 'api';
import config from 'config';
export const foobarRequest = <Data>(
request: ApiRequestDetails<Data>
): ApiRequest<Data> => ({
baseURL: config('foobarBaseUrl'),
withCredentials: true,
...request,
}); // src/api/foobar/user/search
import { foobarRequest } from 'api/foobar';
import { GetRequest } from 'util/useRequest';
import { User } from '.';
export type SearchResult = User;
export const getSearchRequest = (term: string): GetRequest<SearchResult[]> =>
term
? foobarRequest({
url: '/api/user/search',
params: { term },
})
: null; The Api types are just subsets of the Axios request and response types. |
Used in search component like this: const { data: searchResult, isValidating } = useRequest(getSearchRequest(term)); The |
@quietshu
The major issue with it being an
EDIT: |
I suppose? Want me to try put together a PR? |
Btw, re your second point about minimizing memory footprint, I'm pretty sure it's not really duplicating anything. The returned |
@Svish you are not passing a reference, but creating a new object. Hence, duplicating data.
Regarding a PR, I'm also fine creating it, just want an approval from the maintainers to write the proposal. |
@Svish |
@Svish EDIT: reminder, the axios response includes several properties, so we introduce confusion, duplicating the reference |
Aha, well, if you do get approval feel free to make the proposal. Just pay attention to the typings, and if at all possible, make it possible to override them. The axios types are quite "big" and allow a lot of things, which might be good to hide. For reference, here's what we actually use in our current project where I use this // api/index.ts
import axios, { AxiosRequestConfig, AxiosResponse, AxiosError } from 'axios';
/**
* A bare minimum subset of AxiosRequestConfig,
* which also leaves out method forcing it to be
* a GET request, which is the default.
* Also note the unused <Data> type which is there
* so the request can define its return data type.
*/
export interface ApiGetRequest<Data>
extends Pick<
AxiosRequestConfig,
'baseURL' | 'headers' | 'params' | 'data' | 'withCredentials'
> {
url: string;
}
/**
* Adding in the method for use in other cases than useRequest
*/
export interface ApiRequest<Data> extends ApiGetRequest<Data> {
method?: 'DELETE' | 'POST' | 'PUT' | 'PATCH';
}
/**
* A bare minimum subset of AxiosResponse
*/
export interface ApiResponse<Data = unknown>
extends Pick<AxiosResponse, 'status' | 'statusText' | 'headers'> {
data: Data;
}
/**
* A bare minimum subset of AxiosError
*/
export interface ApiError<Data = unknown> extends Pick<AxiosError, 'message'> {
response?: ApiResponse<Data>;
}
/**
* Axios wrapped to accept and return the subset types
*/
export function apiRequest<Data = unknown>(
request: ApiRequest<Data>
): Promise<ApiResponse<Data>> {
return axios(request);
} // util/useRequest.ts
import useSWR, { ConfigInterface, responseInterface } from 'swr';
import { apiRequest, ApiGetRequest, ApiResponse, ApiError } from 'api';
/**
* An `ApiGetRequest`, or `null` if no request is to be performed (e.g. in the case of an empty search term)
*/
export type GetRequest<Data> = ApiGetRequest<Data> | null;
/**
* Adjusted useSWR return type, which corrects the data and response keys
*/
interface Return<Data, Error>
extends Omit<responseInterface<ApiResponse<Data>, ApiError<Error>>, 'data'> {
data: Data | undefined;
response: ApiResponse<Data> | undefined;
}
/**
* Adjustment of the useSWR ConfigInterface to make initialData
* work the same, i.e. so you can pass in just the data, and not
* a full axios response, which useSWR would expect
*/
export interface Config<Data = unknown, Error = unknown>
extends Omit<
ConfigInterface<ApiResponse<Data>, ApiError<Error>>,
'initialData'
> {
initialData?: Data;
}
export default function useRequest<Data = unknown, Error = unknown>(
request: GetRequest<Data>,
{ initialData, ...config }: Config<Data, Error> = {}
): Return<Data, Error> {
const { data: response, ...returned } = useSWR<
ApiResponse<Data>,
ApiError<Error>
>(
request && JSON.stringify(request),
// @ts-ignore Typescript complains that request can be null, but this won't be called if it is
() => apiRequest(request),
{
...config,
// If we have initial data, wrap it in a simple fake axios response
initialData: initialData && {
status: 200,
statusText: 'InitialData',
headers: {},
data: initialData,
},
}
);
return {
...returned,
data: response && response.data,
response
};
} EDIT: "Flipped" the |
Re the reference thing, yeah, I was pretty sure it was just a reference 😛 Re the axios response, yeah, it includes several things, which might be useful or not, but I would prefer to leave it as it is. I pull out a reference to |
Thanks for the suggestions everyone 👍 I think custom hooks are a good idea here for my use case |
@Svish
Instead of discussing your proposal, I'll just leave my unopinionated version that doesn't decide for the user how to interact with import axios, { AxiosError, AxiosRequestConfig, AxiosResponse } from "axios"
import useSWR, { ConfigInterface } from "swr"
export const setUseSWRAxios = (
initialRequest?: Omit<AxiosRequestConfig, "method" | "data">,
initialConfig?: ConfigInterface,
) => {
return function useSWRAxios<Data = unknown, Error = unknown>(
/**
* it's possible to add "| keyof InitialRequest or | keyof InitialConfig"
* but we should leave the ability to override global config
*/
request: Omit<AxiosRequestConfig, "method" | "data">,
config?: ConfigInterface,
) {
const modifiedRequest: AxiosRequestConfig = {
...initialRequest, // for global customizations
...request,
method: `GET`,
}
const modifiedConfig: ConfigInterface = {
...initialConfig, // for global customizations
...config,
}
/**
* Axios has a wrapper object around data => filter response
*/
const { data: response, ...responseUseSWR } = useSWR<
AxiosResponse<Data>,
AxiosError<Error>
>(
JSON.stringify(modifiedRequest),
async () => axios(modifiedRequest),
modifiedConfig,
)
const { data, ...responseAxios } = response || {}
return { ...responseUseSWR, responseAxios, data }
}
} You could then use it as follows: const useSWRAxios = setUseSWRAxios(
{ baseURL: `https://google.com` },
{ revalidateOnFocus: false },
)
function Profile () {
const { data, error } = useSWRAxios({ url: `/some-url` })
if (error) return <div>failed to load</div>
if (!data) return <div>loading...</div>
return <div>hello {data.name}!</div>
} |
@o-alexandrov Yeah, of course I wasn't expecting such limited types to be used in a generic hook like this. The original Axios types are fine for that, but I just wondered if it would be possible to allow for overriding the types in the Anyways, comments to your proposal:
|
I really like the idea of
Here's my proposal, and I think it's more general, and works better for all libs & scenarios: #172 |
The problem I see with this kind of custom hooks that wrap a specific I'm playing with the idea of returning something like import useSWR, { mutate as swrMutate } from 'swr';
function useData(endpoint /*, ... */) {
const key = [endpoint /* , ... */];
const mutate = (data, shouldRevalidate) => swrMutate(key, data, shouldRevalidate);
const { data, error, revalidate, isValidating } = useSWR(key /*, ... */);
return {
data,
error,
revalidate,
isValidating,
mutate,
key: [...key]
};
}
export default useData; You could take this idea one step further and create yet another hook for the key generation bit. That way, if you need to // use-users.js
import useSWR, { mutate as swrMutate } from 'swr';
const isNil = value => value === null || value === undefined;
export function useKey(token /*, ... */) {
return isNil(token) ? null : ['/users', token.trim() /*, ... */];
}
export function useMutate(token /*, ... */) {
const key = useKey(token /*, ... */);
return (data, shouldRevalidate) => swrMutate(key, data, shouldRevalidate);
}
function useUsers(token /*, ... */) {
const key = useKey(token /*, ... */);
// ...
const { data, error, revalidate, isValidating } = useSWR(key, fetchWithToken);
return {
data,
error,
revalidate,
isValidating
};
}
export default useUsers; Any thoughts on this? |
@nfantone
Regarding the proposal of making the mutate function as part of the response of the |
@o-alexandrov Seems like #136 got implemented through #245. I didn't know about those. Good stuff! However, the way I see it, there's still an issue with locality: the returned, partially applied, |
The docs still say one should not use objects as keys. So whats with that now? I also see several tests that use object keys now. |
I did something similar with |
Update: I’m planning to add key serialization and object key support in #1429. |
Added to useSWR({ query: graphql`...`, variables }, request)
useSWR({ query: graphql`...`, variables }, request)
// ^recognized as the same resource |
So it does deep compare on the object fields? |
@pke yes, same for array keys. |
Would it be possible to support objects as keys rather than forcing everything to be a string?
In my case I'm looking to do something like
which returns the same object given the same inputs. The object itself has a method on it that is called by the
fetcher
:As it is this doesn't work because the object returned is forced to a string here: https://github.com/zeit/swr/blob/master/src/use-swr.ts#L66
Given the cache is a
Map
using an object as a key works, eg. changing that line to the following appears to do what I want:Wrapping it in an array does also work:
but the ergonomics aren't as good (especially given if you forget to wrap it in an array it just doesn't work - no errors).
Unfortunately other things assume it's a string as well - eg.
CONCURRENT_PROMISES
etc are just plain objects rather than aMap
.Is this something that you would be willing to extend the API to accomodate?
My alternative to make this work within the current API constraints would be to have
prepare
return a string thatfetcher
can look up into my own global cache... but I was thinking it would be nice to avoid this ifswr
already did that internally.The text was updated successfully, but these errors were encountered: