square / svelte-store

528 stars 36 forks source link

Typescript support for initial `undefined` state #61

Closed ngalaiko closed 1 year ago

ngalaiko commented 1 year ago

Example:

const getData = (): Promise<Data[]> => { /* ... */ }
const store = asyncDerived([], getData)
<script lang="ts">
    import {store} from '$lib/stores'
    import {derived} from '@square/svelte-store'
    const transformedData = derived(store, data => {
        // according to TypeScript, data here has type of Data[], when in reality it's Data[] | undefined
    })
</script>

is there any way to enforce undefined checks inside the derived stores?

one way of doing it that i have in mind is by defining a wrapper similar to how nanostores suggest to do it:

import { atom, onMount } from 'nanostores'

type Posts = { isLoading: true } | { isLoading: false, posts: Post[] }

let posts = atom<Posts>({ isLoading: true })

onMount(posts, () => {
  fetch('/posts').then(async response => {
    let data = await response.json()
    posts.set({ isLoading: false, posts: data })
  })
  return () => {
    posts.set({ isLoading: true })
  }
})

how would you approach this?

Akolyte01 commented 1 year ago

Hey! Could you elaborate on what you mean by "enforce undefined checks"?

according to TypeScript, data here has type of Data[], when in reality it's Data[] | undefined

per your example I don't think it should be. store initializes with a value of [] and then updates to the results of getData. The data the derived store receives shouldn't be undefined unless getData can return undefined.

Screenshot 2023-05-05 at 12 50 03 PM

If you have strict mode on the derived store will correctly complain about a parent's value possibly being undefined before it has finished deriving. Which can be fixed by using $myParent?.myProp instead.

ngalaiko commented 1 year ago

@Akolyte01 hi!

your example is exactly what i mean, but for asyncWritable stores. If I start off with an asyncWritable, TypeScript doesn't see the undefined case, however, it happens.

Screenshot 2023-05-08 at 08 45 57

my use-case is as follows:

Akolyte01 commented 1 year ago

Oh I see... Yeah I got confused about the [] first argument and thought that was the initial value, not the array of parent stores. Sorry!

Looking into it I think this exceeds my current typing abilities with TypeScript. I think the root of the issue was that the package was not written in strict mode and as such the typing may act unexpectedly when it comes to undefined values.

I've made an issue #62 to first address, then I can double back to this with hopefully a better understanding on how to deal with this.

Akolyte01 commented 1 year ago

Aha.... Actually I found the source of the issue. This is inherited from a base svelte/store typing problem.

const initial = Math.random() > 0.5 ? { myProp: 'initial' } : undefined;
const myParent = writable(initial, (set) => {
  Promise.resolve({ myProp: 'updated' }).then((value) => set(value));
});
const myDerived = derived(myParent, ($myParent) => $myParent.myProp);

The type of myParent is Writable<{myProp: string}> instead of Writable<{myProp: string} | undefined>`

looking farther into why that's the case...

Akolyte01 commented 1 year ago

Okay, extra weird: this problem with the default readable/writable stores only exists in typescript 4 and works as expected in typescript 3....

ngalaiko commented 1 year ago

@Akolyte01 here is what I found: https://github.com/sveltejs/svelte/issues/6291

it looks to me that if initial value is undefined, type system treats it as if there is no value set at all.

I think the actual error comes from this library, when derived callback is executed before the first writable set in called

but instead, derived stores must not run the callback until all of the dependencies have value set

Akolyte01 commented 1 year ago

That would be the desired behavior if we were starting this library from scratch, but unfortunately there's some prior behavior that we need to maintain compatibility with.

The 'default' stores in this library are all written as extensions of the existing svelte/store package. The intention here is that people can switch over to using this library from the base without breaking anything. This creates some limitations to how much we can modify the behavior of derived stores, for example. The ideal thing for this package would be to do as you said: to not have derived stores perform their derivation until their parents have loaded. However this would break feature parity with the original svelte/store.

it looks to me that if initial value is undefined, type system treats it as if there is no value set at all.

It gets a little more complicated because the behavior changes depending on whether you are using typescript 3 or 4. For example check here: https://codesandbox.io/p/sandbox/svelte-typescript-forked-5b6jz0

This is a typescript 4+ project, and as you can see the derived store does not recognized that $myParent is potentially undefined. However if you switch over the project to typescript 3 (what svelte is written with) then the typing starts working as expected; $myParent is highlighted as potentially undefined.

This problem effects this library because asyncWritable stores use a writable store internally, so the typing of that 'subscribe' function is what informs the final typing of the store.

ngalaiko commented 1 year ago

thanks for looking into that! downgrading typescript is acceptable for my case, and it kind of makes sense given that svelte itself is using typescript v3. maybe in the future there is a better way to resolve it.

ngalaiko commented 1 year ago

if someone is interested, i've made my own alternative with desired (by me) behaviour https://github.com/ngalaiko/svelte-loadable-store

Akolyte01 commented 1 year ago

Cool! FWIW, I think you could also achieve the desired behavior by using an asyncDerived store, which will wait for all parents to load before running its own load callback.

Akolyte01 commented 1 year ago

This bug may be fixed when svelte updates to a newer version of typescript. Closing until then.