Open ianstormtaylor opened 3 years ago
Hmm, the following don't seem all that different...
const {state, contents} = useRecoilValueLoadable(...);
if (state === 'hasError') { ... }
vs
const {data, isError, error} = useRecoilValueLoadable(...);
if (isError) { ... }
For the "90% of cases where T
is either a value or undefined
," Loadable
has a few convenience functions if you don't care about managing all of the states directly. e.g.
const value = useRecoilValueLoadable(...).valueMaybe();
return <div>{value ?? 'fallback'}</div>;
But managing all of the states isn't that verbose:
const loadable = useRecoilValueLoadable(...);
return {
hasValue: <div>hello {loadable.contents}!}</div>
hasError: <div>failed to load...</div>,
loading: <div>loading...</div>,
}[loadable.state];
You could also create your own hook to do exactly that:
export default function useLoadable(state: RecoilState) {
const loadable = useRecoilLoadable(state);
return {
loading: loadable.state === 'loading',
error: loadable.state === 'hasError' ? loadable.contents : undefined,
data: loadable.state === 'hasValue' ? loadable.contents : undefined,
};
}
Apologies for typos and misremembering the syntax, I am writing this on my phone.
@drarmstr sorry, I should have explained further. I'm not just talking about terseness here, but about making the syntax easier for people to understand. I think the current API has a more convoluted mental model, which makes it harder to understand/read, and results in buggier code. It causes a few different issues…
It's harder to ensure you've handled all the states. You have to make sure you've written an if
(or ternary) for each of the state
values. Or you have to use a switch
which are not easy to use inline in JSX, especially in nested children. This is more error prone than ensuring you're using both data
and error
(especially since ESLint will warn for unused variables).
It's harder to quickly check loading/error states. You have to check loadable.state === 'hasError'
, and then you have to read loadable.contents
(and mentally remember that that property changes depending on the current state). This is more convoluted than just testing error
and using it.
Harder to rename properties when using multiple loadables. You have to choose to either prefix like userLoadable
and then have even longer variable usage everywhere. Or you have to prefix the awkward sub-properties like userState
and userContents
which are not obvious concepts. This is much more convoluted than prefixing as user
and userError
.
I'd encourage you to use GitHub's code search to see how people are using the API. I think it reveals a lot of the extra confusion the mental model of state, contents, valueMaybe, errorOrThrow, ...
causes, instead of the simpler data, error
. There are a lot of things people are doing weirdly…
switch
statements which JSX doesn't handle. (1, 2, 3, 4, 5, 6, 7)This could be solved with a single change—exposing error
and data
properties, instead of the current merged contents
property. I don't think so many other libraries have ended up with this API by accident, but because it's much easier for people to understand. (Check out the other libraries's GitHub code search results too, often much clearer.)
And it has some nice safeguarding properties too:
const { data, error } = useRecoilLoadableValue(...)
// If you forget this error check, you'll get an ESLint error for
// the unused `error` variable, saving you.
if (error) return <Error />
// If you forget this loading check, you'll get a TypeScript error
// for accessing data when it could be undefined, saving you.
if (!data) return <Loading />
return data.map(...)
And it provides a clear set of renames when using multiple loadables at once:
const { data: user, error: userError } = useRecoilLoadableValue(...)
const { data: teams, error: teamsError } = useRecoilLoadableValue(...)
I'm not arguing for getting rid of the state
, since I see the value in that discriminating ability. But I don't think merging all the results into contents
is a good approach.
Well, you'll only get an unused variable lint warning about not checking error
if you remember to destructure error
. But more importantly, a check like if (error)
is not safe because any value could thrown or rejected by a promise, including falsey values like null
and undefined
. So, to be able to handle any type for the data or error type you also need an out-of-band signal for the state.
But more importantly, a check like if (error) is not safe because any value could thrown or rejected by a promise, including falsey values like null and undefined. So, to be able to handle any type for the data or error type you also need an out-of-band signal for the state.
@drarmstr Can you explain this? Not sure I understand. Could you not just wrap with an internal error if a non-Error
object is thrown? Throwing non-Error
objects is frowned upon in JS generally, so this seems like a rare issue? (Except for when suspending of course.)
It may be frowned upon, but it is still possible in JS to throw arbitrary values or reject Promises with arbitrary values and we need to be able to handle that case.
@drarmstr yeah but…
Error
thrown values could be wrapped by Recoil automatically.if
stronger (since they know their domain).error == null
.There are lots of ways to solve that specific issue, not sure why that would prevent this API improvement. (My examples are from very popular existing libraries, so they've clearly figured out the workable tradeoff.)
Sorry to necro a super old thread but I'm having this same discussion with another dev.
IMO useRecoilStateLoadable
and useRecoilValueLoadable
feel clunky to work with.
It feels like we're writing code that should be happening under the hood of Loadable
.
const loadable = useRecoilValueLoadable(activeValue);
const value =
loadable.state === 'hasValue'
? loadable.contents?.thingINeed
: null;
Maybe our current implementation/usage of Loadables is incorrect, if so please tell me!
Sorry to necro a super old thread but I'm having this same discussion with another dev.
IMO
useRecoilStateLoadable
anduseRecoilValueLoadable
feel clunky to work with.It feels like we're writing code that should be happening under the hood of
Loadable
.const loadable = useRecoilValueLoadable(activeValue); const value = loadable.state === 'hasValue' ? loadable.contents?.thingINeed : null;
Maybe our current implementation/usage of Loadables is incorrect, if so please tell me!
For your example the valueMaybe() accessor is similar, though uses undefined instead of null.
I have borrowed the concept of Loadable from Recoil and adapted it for my workplace, and it has undergone some evolution throughout the years.
https://gist.github.com/stylemistake/d151e26de021a3afca33125abb1c1047
I found that having a contents
field resulted in very verbose code, and required value
and error
methods/getters just to get a value and an error, and by switching the implementation to directly storing value in the value
field and error in the error
field made the implementation a lot simpler, both in JS code itself and types.
It also makes it pretty straightforward to destructure what you want via const { value, error }
, even though we don't use this syntax extensively. More frequently we just use the loadable directly, like this:
const user = useSelector(selectUser);
return <span>{user.value?.name ?? 'no name'}</span>;
It also makes visual debugging easier since you can immediately see that a Loadable has a value
or an error
field, and diffs in Redux really help visualising it, e.g. value got removed and error was added, vs. having a change in contents
field.
It does not make typing harder, as you can see from the code.
You can use state checks to narrow down the type of loadable, like so:
if (loadable.isValue()) {
// This is now legal, because type of `value` is now known
// and can't be `undefined`.
loadable.value.x
}
I don't believe reusing a contents
field amounts to any performance or memory gains, it has never been an issue for us, and we never had a need to specifically benchmark this.
So hopefully this gives you the feedback you wanted. I think it's the right way to move forward, it should map pretty well onto existing API (you just need to add getters for value
and error
and make them smartly typed). And we're also interested in the ways we could refine the concept further.
We additionally found a very good case for having a Loadable.initial
state, which is a state when there is nothing at all, and it isn't loading either - like an uninitialized variable, or a true "null" state like in Option
or Nullable
monads in other languages. We frequently use this state to know when a loadable resource has never been loaded, and we fetch the resource right after, becoming Loadable.loading
.
Of course this can be replaced with Loadable | null
, but then if you wanted to access a value you'd have to use too many null property checks loadable?.value?.x
which is a bit too cumbersome.
@stylemistake - A limitation of using separate value
and error
properties is that we also need to support the situation where either a valid value or error may be nullable. So syntax like user.value?.name ?? 'no name"
isn't completely sufficient.
@drarmstr It's not a limitation, state can still be accessible and you can discriminate between them the exact same way. it's just that common use cases like the above are made easier.
Right now working with
Loadable
values is pretty tedious. It would be really nice if the API could be simplified for the 90% of common cases where you just want to know if something is loading, errored, or has data.This is a common pattern that has been solved in nicer ways in existing libraries. For example in SWR with
useSWR
:Or in Apollo with
useQuery
:Or slightly more verbosely in React Query with
useQuery
:For 90% of cases where the data will be either
T
orundefined
when still loading, I think SWR's minimal API is the nicest. Either you have anerror
, or you havedata
or you're still loading. (And for the edge cases where data can resolve successfully toundefined
you can use the deeper API.)It would be really nice to do something like:
Instead of the current API where you have to wrangle with the awkward bits like
loadable.status === 'hasValue'
orloadable.errorOrThrow()
.