square / svelte-store

528 stars 35 forks source link

Existing inline functionality is not reactive #25

Closed chrischen-sq closed 2 years ago

chrischen-sq commented 2 years ago

Example: {#await safeLoad(myStore) then loadedSuccessfully} This generates a single promise that resolves on the first load of the store. If that resolves successfully but then the store is reloaded and fails, the template does not have any access to this new state information.

Akolyte01 commented 2 years ago

Here is the current workaround pattern:

myLoadState = writable();

myAsyncStore = asyncReadable(
  null,
  async () => {
    myLoadState.set('loading')
    try {
      const storeValue = await someAsyncProcess;
      myLoadState.set('loaded') ;
      return storeValue;
    } catch (error) {
      myLoadState.set('errored');
      throw error;
    }
  },
  true,
);

However it would be nice to not have to manually create and manage that load state store every time this functionality is needed.

smblee commented 2 years ago

would be a nice API to be able to access this state like myAsyncStore.state where the state could be idle | loading | success | error. Bonus points if we can capture the "reloading" aspect, to support SWR (stale while revalidate) behavior, like idle | loading | reloading | success | error

Akolyte01 commented 2 years ago

That would be ideal. Where I've run into blockers while pursuing that is keeping myAsyncStore.state reactive and easily useable in templates without adding a bunch of weight to stores that don't need that functionality.

smblee commented 2 years ago

@Akolyte01 hmm do you think it's adding much weight though? can't people opt out of it just by not using the .state? and i think Loadable inherently should have .state since it covers async data so it makes sense to be part of the obj

smblee commented 2 years ago

actually the access pattern will need to be a bit different to access both data & state easily... accessed at second layer something like

const { data: myItem, state, error } = myStore;
...

{#if $state === 'loading'}
loading...
{:else if $state === 'success'}
<div>{$myItem}</div>
{/if}
smblee commented 2 years ago

really just sketching it out here... but borrowing the API concept from react-query and building on top of svelte-store...

I also added another wrapper called useQuery which isn't necessary, but it allows you to key things by arbitrary string, allowing you to create caches for individual items like post.123 or post.234. But I consider this more of higher level helper fn, and not really related to current discussion on state. constructStore function is more inline with our current discussion.

anywho, here's what i came up with so far.


const createAsyncStateStore = () => writable<'fetching' | 'paused' | 'idle'>('idle');

const storeMap: Record<string, AsyncReadable<unknown>> = {};

function getStore<T>(key: string) {
    return storeMap[key] as AsyncReadable<T>;
}

function useQuery<S extends Stores, T>(
    key: string,
    stores: S,
    fn: (values: StoresValues<S>) => Promise<T>,
    initialValue?: T
) {
    if (getStore(key)) return getStore<T>(key);
    const store = constructStore(stores, fn, initialValue);

    storeMap[key] = store;

    return store;
}

function constructStore<S extends Stores, T>(
    stores: S,
    fn: (values: StoresValues<S>) => Promise<T>,
    initialValue: T
) {
    const errorStore = writable();
    const dataStore = writable(initialValue);
    const fetchStateStore = createAsyncStateStore();
    const stateStore = derived(
        [errorStore, dataStore, fetchStateStore],
        ([$error, $data, $fetchState]) => {
            if ($data) {
                return 'success';
            } else if ($error) {
                return 'error';
            } else if ($fetchState === 'fetching') {
                return 'loading';
            }

            return 'loading';
        }
    );

    const derivedStore = asyncDerived(
        stores,
        async (values) => {
            try {
                errorStore.set(null);
                fetchStateStore.set('fetching');
                const value = await fn(values);
                dataStore.set(value);
                return value;
            } catch (error) {
                fetchStateStore.set('idle');
                errorStore.set(error);
                throw error;
            }
        },
        true,
        initialValue
    );

    return {
        data: { subscribe: dataStore.subscribe },
        load: derivedStore.load,
        reload: derivedStore.reload,
        state: { subscribe: stateStore.subscribe },
        fetchState: { subscribe: fetchStateStore.subscribe }
    } as AsyncReadable<T>;
}

type AsyncState = 'loading' | 'success' | 'error';
type FetchState = 'fetching' | 'idle';
type AsyncReadable<T> = {
    data: Readable<T>;
    load: () => Promise<T>;
    reload?: () => Promise<T>;
    state: Readable<AsyncState>;
    fetchState: Readable<FetchState>;
};

and consume it like

// store.ts
export const usePost = (postId: string, initialValue?: Post) =>
    useQuery(
        `post.${postId}`,
        [currentProfileStore],
        async ([$currentProfile]) => {
            return Api.getPost($currentProfile.id, postId);
        },
        initialValue
    );

// Page.svelte
{ data, load, reload, state, fetchState } = usePost('123');
Akolyte01 commented 2 years ago

So I think we're aligned on basic concept, though I think in exploring this you've run into the same issues I have. First being that making the state reactive doesn't work well with existing svelte syntactic sugar. Requiring destructuring makes using these custom stores harder to use and brings them out of line with the established patterns of the vanilla stores. I definitely don't want to make the package harder to use to get this feature in.

The second is weight. Here you are creating multiple internal stores and multiple internal subscriptions to power this logic. I ran into similar issue. So if all of the loadable stores include this functionality you'd end up doubling or tripling the amount of stores required to power an app.

I think there may be a solution here using a meta programming approach, which would allow for opting into this functionality on a store by store basis.

As an example of the general direction I'd like to go in

// still let people create and use stores as is
const myStore = asyncReadable( // some arguments //);

// also provide a way to opt in to state tracking
const { store: myStore, state: myStoreState } = trackState( asyncReadable( // some arguments //) );

Although maybe that's overcomplicating things--providing a construction parameter to opt in may be preferable.

smblee commented 2 years ago

Awesome that we are thinking towards the same direction - definitely agree with your points on all front :)

Couple thoughts here:

  1. Hopefully nested store access will get better through more rich syntactic sugar around reactive assignment. But we dont have clear idea when such a thing will come so we can only hope 🤞 (https://github.com/sveltejs/svelte/issues/6373#issuecomment-851059020) (https://github.com/sveltejs/svelte/issues/4079)

  2. Is weight of the extra states that big of an issue? Is the weight negligible even at a decent scale, and the benefits outweigh (heh) the weight itself? Might be worth doing some benchmark here

  3. If weight does matter, i think having some measure of opt-in/opt-out might be desirable, but that could also complicate things as it might be harder to use composition of these stores. Having the same API across might prove more useful. Or we could treat the "async state management" as a layer on top. It could be similar to trackState you mentioned in your example code but could also be simple "key"'s that encapsulates the underlying stores similar to my example. Will have to think a bit more about this though, especially from angles of composition, and ease of use.

Akolyte01 commented 2 years ago

As a heads up this feature is in beta testing at the moment and should be released soon!

smblee commented 2 years ago

@Akolyte01 already saw :) very excited!

Akolyte01 commented 2 years ago

Okay took longer than expected but 1.0.10 is now out! This addresses this use case with the trackState feature.