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

Better Typing support when suspense is enabled #1412

Open
huozhi opened this issue Aug 30, 2021 · 17 comments
Open

Better Typing support when suspense is enabled #1412

huozhi opened this issue Aug 30, 2021 · 17 comments

Comments

@huozhi
Copy link
Member

huozhi commented Aug 30, 2021

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.

@FDiskas
Copy link

FDiskas commented Sep 6, 2021

@shuding
Copy link
Member

shuding commented Sep 6, 2021

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.

@Svish
Copy link
Contributor

Svish commented Sep 6, 2021

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.

@karol-majewski
Copy link

karol-majewski commented Sep 7, 2021

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

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

@Svish
Copy link
Contributor

Svish commented Sep 7, 2021

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

@shuding
Copy link
Member

shuding commented Sep 7, 2021

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)

@karol-majewski
Copy link

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

@MoSattler
Copy link

MoSattler commented Jan 6, 2022

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?

@Svish
Copy link
Contributor

Svish commented Jan 6, 2022

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.

@MoSattler
Copy link

MoSattler commented Jan 6, 2022

@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
}

@karol-majewski
Copy link

@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.

@MoSattler
Copy link

MoSattler commented Jan 7, 2022

@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.

@karol-majewski
Copy link

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.

@MoSattler
Copy link

MoSattler commented Jan 7, 2022

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.

@WalterWeidner
Copy link

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

@andyrichardson
Copy link

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;
}

@filiptrplan
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants