swr icon indicating copy to clipboard operation
swr copied to clipboard

Better Typing support when suspense is enabled

Open huozhi opened this issue 2 years ago • 17 comments

Some restrictions can be applied to swr when suspense is enanbled:

  • key can’t be falsy
  • fetcher can’t be null
  • data can’t be undefined

Note: this is tricky as TypeScript can’t handle inherited configs, and those can be dynamic in the runtime.

huozhi avatar Aug 30 '21 17:08 huozhi

Already fixed in https://github.com/vercel/swr/releases/tag/1.0.1-beta.0 ???

FDiskas avatar Sep 06 '21 06:09 FDiskas

No it’s not added yet. I don’t think this can be handled by TypeScript because configs can be inherited from SWRConfig. What we can do temporarily is to support the per-hook configuration.

shuding avatar Sep 06 '21 09:09 shuding

My suggestion would be to just separate the two. Either a separate hook, or just a separate import like from swr/suspense or something like that.

That way you could simplify the regular hook by removing all suspense related stuff from there, and the suspense hook could be written with perfect types and no non-suspense related stuff.

Svish avatar Sep 06 '21 12:09 Svish

What should happen if useSWR from swr/suspense was used alongside a global config contradicting it?

<SWRConfig value={{ suspense: false }}>
  <App />
</SWRConfig>

karol-majewski avatar Sep 07 '21 06:09 karol-majewski

Can't you already do that? Doesn't useSWR currently take suspense as an option, which overrides the default config?

Svish avatar Sep 07 '21 08:09 Svish

By definition, useSWRSuspense should override the global config of suspense: false.

Also, it would be great to cover more use cases such as waterfall (#5, #168) and dependents with this suspense hook.

Waterfall:

useSWRSuspense(key1)
useSWRSuspense(key2)

Dependent (should it fallback to the suspending state if the first resource changes upon focus?):

const { data } = useSWRSuspense(key1)
useSWRSuspense(data.id)

shuding avatar Sep 07 '21 14:09 shuding

OK, so your proposal is for the new useSWR to override default settings with { suspense: true }. That makes sense.

How should then conditional fetching be solved? Suspense users should also be allowed to make conditional requests, therefore key cannot be required to be truthy. If key can be falsy, then that implies data will sometimes be defined, sometimes not. This doesn't leave much room for improvement on the type level.

We can, however, leverage safestring to determine when Suspense-powered requests are guaranteed to deliver a response every time — which is when key is guaranteed to be truthy. Suggested improvement (simplified for brevity):

declare function useSWR<T>(key: string): { data: T  };
declare function useSWR<T>(key: string | null): { data: T | undefined };

declare function safestring(strings: TemplateStringsArray, ...args: Array<string | number>): string;
declare function safestring(strings: TemplateStringsArray, ...args: Array<string | number | null | undefined>): string | null;
declare function safestring(strings: TemplateStringsArray, ...args: Array<string | number | null | undefined>): string | null;

declare const customerId: string | null;

useSWR<Response>(safestring`/api/v1/customers`).data;              // Response
useSWR<Response>(safestring`/api/v1/customers/${customerId}`).data // Response | undefined

TypeScript playground

karol-majewski avatar Sep 07 '21 14:09 karol-majewski

Is there any workaround right now, where I can tell SWR that the data will be guaranteed to be defined, thanks to suspense and error boundaries?

MoSattler avatar Jan 06 '22 11:01 MoSattler

Is there any workaround right now, where I can tell SWR will be guaranteed to be defined, thanks to suspense and error boundaries?

Best solution now is probably to make your own typed wrapper hook of useSWR which makes sure things work the way you want. We've done this for react-query, works well.

Svish avatar Jan 06 '22 11:01 Svish

@karol-majewski

If key can be falsy, then that implies data will sometimes be defined, sometimes not.

I don't know the internals of useSWR, but couldn't one in principle use overloading to infer if data is defined or not? Something equivalent to this?

function useSWR<Data>(key:undefined, fetcher: Function): {data: undefined};
function useSWR<Data>(key:string, fetcher: Function): {data: Data};
function useSWR<Data>(key:string | undefined, fetcher: Function): {data: Data | undefined} {
    // code
}

MoSattler avatar Jan 06 '22 11:01 MoSattler

@MoSattler There is a problem with such an overload. Reason: key being a string is not enough to infer that data will be defined.

Consider this common use case:

useSWR(`/customers/${customerId}`, fetcher)

If customerId is nullish, the key is indeed a string, but the fetch call loses its meaning and as such should not be made. Therefore, data should not be expected to be defined even though the key is a string.

karol-majewski avatar Jan 06 '22 17:01 karol-majewski

@karol-majewski

If customerId is nullish, the key is indeed a string, but the fetch call loses its meaning

How so? Wouldn't the string evaluate to '/customers/null', and SWR would make a call to that non-existing endpoint, propagating the 404 error to be handled by the error boundary? At least that seems to be the case for me.

MoSattler avatar Jan 07 '22 08:01 MoSattler

How so? Wouldn't the string evaluate to '/customers/null', and SWR would make a call to that non-existing endpoint, propagating the 404 error to be handled by the error boundary?

Why make a call you know will fail?

Is this current useSWR behaviour, that in this case the fetch call is not made?

It is not, and that the reason why we have #1247 in the first place.

karol-majewski avatar Jan 07 '22 09:01 karol-majewski

It is not, and that the reason why we have #1247 in the first place.

I believe OP and myself are asking for the typing to properly reflect the current implementation, not possible future changes that may or may not come.

In the current implementation it seems to be true that data cannot be undefined with suspense, and that is not correctly reflected in it's typings, and that ought to be fixed.

MoSattler avatar Jan 07 '22 14:01 MoSattler

Is there any way to get some movement on this? Seems the discussion died down.

WalterWeidner avatar Jul 05 '22 14:07 WalterWeidner

Is this issue open to contributions?

I've done a quick test override and something like this seems to work fine. Added bonus of adding inference for return types + keys passed to the handler.

// swr.d.ts
import { SWRResponse, KeyedMutator } from 'swr';

declare module 'swr' {
  type SWRHook1 = <
    Key extends SWRKey,
    Data extends any,
    Config extends { suspense: boolean }
  >(
    key: Key,
    handler: (arg: Key) => Promise<Data>,
    config: Config
  ) => Config['suspense'] extends true
    ? SWRResponseSuspense<Data>
    : SWRResponse<Data, Error>;

  type SWRResponseSuspense<D> = { data: D, mutate: KeyedMutator<D> }

  declare const _default: SWRHook1;
  export = _default;
}

andyrichardson avatar Dec 09 '22 12:12 andyrichardson

This looks like a great feature to be used in the new React 18 update! It would be great if someone implemented it.

filiptrplan avatar Dec 15 '22 21:12 filiptrplan