solidjs / solid

A declarative, efficient, and flexible JavaScript library for building user interfaces.
https://solidjs.com
MIT License
32.05k stars 914 forks source link

Proposal: `data` with better type elimination from `createResource` #608

Closed lxsmnsyc closed 2 years ago

lxsmnsyc commented 3 years ago
const [data] = createResource(...);

Currently, data returns an accessor that returns the current result of the resource. To check if it is still loading, we use data.loading and use data.error when checking for failures. This is great but not for Typescript.

https://github.com/solidjs/solid/blob/main/packages/solid/src/reactive/signal.ts#L216-L233

The accessor returns T | undefined but that is incorrect for 3 reasons:

There is a way to fix this but would introduce a breaking change:

interface Pending {
  status: 'pending';
  value?: undefined;
}
interface Success<T> {
  status: 'success';
  value: T;
}
interface Failure {
  status: 'failure';
  value: any;
}
type Result<T> = Success<T> | Failure | Pending;

// Either the bottom one or a proxy for Result<T>
type ResourceData<T> = () => Result<T>;

This way, the result is deterministic, exclusive and can be type-eliminated.

const result = data(); // Result<T>;
switch (result.status) {
  case 'pending': // Pending
    return null;
  case 'success': // Success<T>
    return <h1>Success! {result.value}</h1>;
  case 'failure': // Failure
    return <h1>Something went wrong. {result.value.message}</h1>;
}

edit:

interface Pending {
  status: 'pending';
  loading: boolean;
  error: any;
  (): undefined;
}
interface Success<T> {
  status: 'success';
  loading: false;
  error: any;
  (): T;
}
interface Failure {
  status: 'failure';
  loading: boolean;
  error: any;
  (): undefined;
}
interface Stale<T> {
  status: 'stale';
  loading: true;
  error: any;
  (): T;
}

Playground

ryansolid commented 3 years ago

I see. That's interesting. I guess with that API it doesn't even need to be a function accessor. It just has 2 getters. My instant hesitation was I liked the whole feels like a Signal thing.. with Suspense you rarely use the loading or error states.

We can't do that union trick the other way I guess. Like this won't fly?

interface Pending {
  loading: true;
  error: false;
  (): undefined;
}
interface Success<T> {
  loading: false;
  error: false;
  (): T;
}
interface Failure {
  loading: false;
  error: any;
  (): undefined;
}

I'm gathering not or you would have suggested it.

Hmm, it is breaking though which ties my hands I think for now.

lxsmnsyc commented 3 years ago

I think that should work as well! Although I'm not sure if we can correctly eliminate the types (for instance, checking for data.error doesn't really eliminate into Failure). Is it a good thing to add a similar field to data.status? status helps brand the types.

https://www.typescriptlang.org/play?#code/JYOwLgpgTgZghgYwgAgAoRAE1Ac2QbwChkTkBnMOMAVzIC5kByABw2xB0YG5jSAbAPZx2OBgCMBAvhDggepZNCgCoDWQE95pABQBKBtSwQYoCJh4BfQoVCRYiFAGVqCJGTIAeACoA+ArxIKKloGRjIXNzJuAORBYVxxSWlZLRIlFTUQTRi9Bi9La1toeCRkADE4YD5qKBQiBSCaeiZ4KpqIaIU4kUSpGTkY9NVkDVTkXORDTGNTc0IrQjB1VmQAJQhwvjBvPwBeNDZcZAAfZGdXDc9fE-LK6tqeQgQBEApFAA84AFtmaWR9gDyYgAVhAEGAAHRwdzAHAgbR6f5+eB8MgQAA0-galCaoXCF3cjHRMW6CWQKLRxIUQwYFIx810IzIaw21C2HgkfVkPkeZAA7sAwAgABbjCCfH7SCGNWiM+qkBDQlBhCKXRgMeUKEjPV59CGCHDacXfX4QPS6MYWMZiWpwADWY0VaJad3a6qxWu1LzIeoNRolpvNMStMRtMgdMSdytYWFw7s1Wp1PqlfuNkrNugtwcsQA

ryansolid commented 3 years ago

Yeah this shows a lot of my misunderstanding of TS I was thinking keeping things in their own lanes would help typing instead of pushing the error through data. As I was saying loading and error were sort of secondary considerations. I don't want data to hold the error incase people aren't checking for it. I think status is probably fine addition and wouldn't be breaking.

lxsmnsyc commented 3 years ago

I added a TypeScript playground link to demonstrate type eliminations with your idea (+ status). Seems to be behaving as expected.

ryansolid commented 3 years ago

Yeah we can do that I need to figure out which state is the source of truth.. status is probably derived like

{
  get status() {
     return error() ? "failure" : loading() ? "pending" : "success"
  }
}

Question of which strings we should use. Is it "success"? is it "failure"? Sometimes resources will start at "success" I guess. .

Is there a 4th state when it's never run and isn't triggered initially?

lxsmnsyc commented 3 years ago

"pending" is set before the fetcher is run. "success" and "failure" corresponds to both when Promise resolves (the statuses were derived from the Promise spec). I think "pending" is also enough for when the fetcher itself hasn't been evaluated yet. Is there a case scenario for a potential 4th state?

Edit:

const [status, setStatus] = createSignal('pending');

// ...
setStatus('pending');
fetcher().then(
  (value) => {
    if (notDisposed) {
      setStatus('success');
    }
  },
  (value) => {
    if (notDisposed) {
      setStatus('failure');
    }
  },
);
ryansolid commented 3 years ago

Ok yeah because it is just boolean not true. And that's fine because status enough to type narrow ok. Yeah I am onboard for this then. Let's see if anyone has any thoughts. But I think we can get this out in the next minor version I guess.

lxsmnsyc commented 3 years ago

After re-evaluating, I think there is a possible 4th state: "stale", a cross between "pending" and "success". It is loading: true while (): T. A resource becomes "stale" when a refetch is performed while there's an existing "success" value.

rturnq commented 3 years ago

I think this proposal is fine, though I'm not sure about "stale". Before moving forward with it, I think we should examine some realistic use-cases to see how well it works in practice. Switching on a string value using a real JS switch works well but I doubt this would be the way people actually end up using it. More likely I expect people to use JSX <Switch>+<Match>, <Show>, or ternaries. That being the case having to check against a string value ends up being pretty verbose.

function Comp() {
  const [data] = createResource(...);

  return (
    <Switch>
      <Match when={data.status === 'pending'}>
        <h3>Loading...</h3>
      </Match>
      <Match when={data.status === 'failure'}>
        <h3>{data.error}</h3>
      </Match>
      <Match when={true}>
        <div>{data()}</div>
      </Match>
    </Switch>
  );
}

While not a reason to do nothing here, when using the control flow components, type narrowing doesn't really work anyways thought I'm hopeful we may be able to do more with TypeScript 4.4. Should probably try out the beta.

lxsmnsyc commented 3 years ago

"stale" is yet optional, but the current behavior of createResource suggest that it will never be applied.

There's not much in 4.4 that can help us narrow types here. <Show> is the closest one I can think of since it also allows proper type elimination through render prop the last time I checked.

ryansolid commented 3 years ago

That works but it also changes the behavior. It basically keys it since data being passed through can change. Not just boolean. So type narrowing is just hard. Because our control flow isn't even keywords. Im not even sure LSP hacks make sense.

ryansolid commented 3 years ago

I looked at React query they have 4 states as well. "idle", "loading", "success", "error". They consider it idle whenever it is in a don't fetch state ie.. if the query is null in our case. They don't have "stale" but also have a "isFetching". To be fair I use "loading" this way and it's up to the user whether they unmount it. But it appears React query only gives the "loading" status when there is no data available. I'm not sure there needs to be a no data check other than checking for data yourself.

It is interesting because changing the query in Solid doesn't wipe the resource value. We keep it around so I realize our proposed types aren't quite right. We can use "stale" for that I guess. But it also doesn't remove from the "success" state in a sense. So there is a question of whether we cater status for this initial load story because that is not how I've node the independent states.

lxsmnsyc commented 3 years ago

Pending, Success and Failure are all states that I derived from the original Promise behavior. We can consider Idle in SSR context however the values that it represents are completely similar (and behaves similarly) to that of Pending. Pending usually means "the promise is yet to resolve" so far of that resolution cycle is "when to begin fetching".

Meanwhile, Stale is completely derived from stale-while-revalidate strategy (give stale data while fetching new one). Not all data-fetching libraries have this behavior tho, and was completely opt-in by other libraries like React Query and Relay.

lxsmnsyc commented 3 years ago

graphviz (1)

Here's a flowchart.

  1. Pending is the initial state of a resource. It transitions to Success when resolving while Failure when it throws an error.
  2. Success will transition back to Pending during a re-fetch. This is only applicable when the behavior is "network-only" (there's no cache).
  3. Success will transition to Stale if the behavior is stale-while-revalidate.
  4. Stale is similar to Pending except that there's a cached data from the previous Promise.
  5. Stale will transition to Pending if the the resolution time is more of the stale age (this is a completely optional behavior). In React, this behavior is akin to a useDeferredValue/useTransition that takes more than 5 seconds to apply (based on the RAIL model).

Stale is a completely optional behavior. It can be emulated with the right implementation of useDeferredValue.

ryansolid commented 3 years ago

Yeah I think it's interesting because React Query's is____ properties are like the current create resource but their status is closer to what is being proposed here. But if we include stale the success path of the switch like @rturnq posted above includes both success and stale. That's my only concern since it regresses from success which might not be obvious. React Query doesn't. But I do think you are right in that they are separate states.

useTransition handles stale pretty well. useDeferredValue allowing means to inverse the stale propagation. I do think with fine-grained we might be able to find a different way to handle this. Like tell the resource to only trigger on initial load etc.. I think useDeferredValue is super awkward way to detect stale state since we deal with containers rather than values. Our experiments only cemented that I don't think it's the best way but that's tertiary to what is being discussed here.

The main case for idle is fetching on demand. Like when the component loads you don't load it. But then on click load data. It isn't treated like loading since there is no intention perhaps of ever loading.

lxsmnsyc commented 3 years ago

The main case for idle is fetching on demand. Like when the component loads you don't load it. But then on click load data. It isn't treated like loading since there is no intention perhaps of ever loading.

Is there an example case-scenario for this? I do think this works well for call-to-action fetches (e.g. POST) and not for data-derived UI. I may be wrong tho.

Edit: Idle looks like is applicable for this scenario, since createResource itself is an on-demand read.