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

Typescript support for initial undefined state #61

Closed
ngalaiko opened this issue May 4, 2023 · 11 comments
Closed

Typescript support for initial undefined state #61

ngalaiko opened this issue May 4, 2023 · 11 comments

Comments

@ngalaiko
Copy link

ngalaiko commented May 4, 2023

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
Copy link
Collaborator

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
Copy link
Author

ngalaiko commented May 8, 2023

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

  • when created, store fetches initial data
  • new data comes via websockets, and store value is updated
  • one or more of such stores are used to derive more stores

@Akolyte01
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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

@ngalaiko
Copy link
Author

ngalaiko commented May 9, 2023

@Akolyte01 here is what I found: sveltejs/svelte#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
Copy link
Collaborator

Akolyte01 commented May 9, 2023

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
Copy link
Author

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
Copy link
Author

ngalaiko commented May 11, 2023

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

@Akolyte01
Copy link
Collaborator

Akolyte01 commented May 15, 2023

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
Copy link
Collaborator

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

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

No branches or pull requests

2 participants