square / svelte-store

528 stars 35 forks source link

Typescript complains: load() of Loadable<T> may be undefined #8

Closed kuechlerm closed 2 years ago

kuechlerm commented 2 years ago

I have a Typescript problem when recreating your examples.

{#await myLoadable.load()}
[...]

Typescripts complains that load might be undefined. I wonder why the interface is like that.

I would have to wrap each Loadable in an #if, like

{#if myLoadable.load}
  {#await myLoadable.load()}

But that is rather cumbersome.

Love the the idea of the package - any other way I am missing to fix this? Thanks!

Akolyte01 commented 2 years ago

That is a configurable typescript option to complain about that or not. If you wish to keep your typescript configuration the same you can use optional chaining here myLoadable.load?.() https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining

This is a holdover of trying to get type inference to work with derived stores based on parent store types. This likely will not be changed before full 1.0.0 release

kuechlerm commented 2 years ago

Thanks for the quick response. Ignoring this kind of error seems like a bad idea for safety reasons.

And

{#await myLoadable.load?.()}
{:then data}
  {#each data as item} <-- just moved the problem to here... data can be undefined

As it sounds like you do not want to change the interface, the only solutions I see are

// needs to be in a seperate ts file, not a .svelte file, because of some JSX nonsene
export const safeCall = <T>(func?: () => Promise<T>) => func?.() ?? Promise.resolve([] as T[]);
export const safeArray = <T>(array?: T[]) => array ?? [];

// Thing.svelte
{#await safeCall(myLoadable.load)}
// or 
  {#each safeArray(data) as item}

Any other ideas?

Akolyte01 commented 2 years ago

Strict mode is an opt in feature of typescript. It does come with benefits to type safety, but also makes many things more cumbersome. I think the optional chaining will work for you with some adjustment to how you are writing your templates.

{#await myLoadable.load?.()}
{:then data}
  {#each data as item}

Although this is the intuitive way to use load to get data from a store you should avoid this. When writing like this data is not reactive. This doesn't matter if you only intend the data to load once, but for data that updates throughout the lifecycle of your app your components will not update correspondingly.

Instead a similar pattern can be adopted

{#await myLoadable.load?.() then _}
  {#each $myLoadable as item}
  ...

Consistently using this pattern in your templates is the best way to keep them reactive and minimize changes needed if your data layer changes. And, mostly coincidentally, lets you use optional chaining here of the load function without further complaint.

it sounds like you do not want to change the interface

It will be changed, just not for a bit, and I'm unclear on which direction it will be. The ideal world would be for typescript to be able to figure out if a derived store descends from a loadable or reloadable store or not and have a corresponding type. I.e. a store that derives from a Reloadable parent would itself be Reloadable while, a store that derives from only synchronous parents would be neither Loadable nor Reloadable.

However I'm not sure if that's actually possible yet. If not then all derived and async stores will be changed to always include a load and reload function regardless of whether they actually do anything.

Coincidentally I am also considering implementing a safeLoad function that will work similarly, with the added functionality of catching any promise rejections and resolving to a boolean that indicates whether the load was successful or not. This could then be used like this:

{#await safeLoad(myLoadable) then success}
  {#if !success}
    <ErrorBanner/>
  {/if}
    <p>{$myLoadable.title}</p>
  .....

This would prevent promise rejections from being 'unhandled' when used in a template without a 'catch' state.

kuechlerm commented 2 years ago

Thanks a lot! I'll go with using load?.() and $myLoadable ... it seems quite obvious on second thought :).

Just my two cents: I am sceptical about swallowing exceptions with a safeLoad. Its seems to make the construct less expressive and one might like to work with the error object in the :catch branch. But a construct like https://mobily.github.io/ts-belt/api/result might be even better.

Akolyte01 commented 2 years ago
Its seems to make the construct less expressive and one might like to work with the error object in the :catch branch.

The intention is for use when a catch branch isn't appropriate/relevant but you still want any rejections to be handled.

That Result type looks promising however, I'll look into that