Add useSkeleton hook, docs & storybook

[jared-dykstra]

This PR adds a new useSkeleton hook.

Although tiny, this hook has a dramatic impact on the developer experience. This hook is a trivially small monad wrapper around a ternary: rendering either children or a skeleton, depending on the hook's isLoading argument.

This PR adds a Use Skeleton Hook storybook story:

This PR adds the following to README.md

The useSkeleton hook

A useSkeleton hook encapsulates conditional logic to keep your code easier to read.

// Assuming a variable or prop exists with the loading state, use the
// hook to get `withSkeleton`
const { withSkeleton } = useSkeleton(loading)

// A skeleton is rendered in place of `value` if `loading` is truthy
<TableCell>{withSkeleton(value)}</TableCell>

If needed, skeleton options can be provided to useSkeleton

const { withSkeleton } = useSkeleton(loading, { round: true })

...or via withSkeleton()

<TableCell>{withSkeleton(value, { count: 4 })}</TableCell>

[jared-dykstra]

@srmagura - Happy to adopt all of your suggestions. Thanks for the quick response!

[dvtng]

This is an interesting idea, but to me it's not clear why useSkeleton needs to be a hook, or why it's much better than alternate patterns. For example, the hook could easily be replaced with:

const maybeSkeleton = value => loading ? <Skeleton /> : value;

<TableCell>{maybeSkeleton(value)}</TableCell>;

For me, this one-liner is more explicit and unambiguous. It's also more flexible. E.g. if in your data-fetching architecture, loading is implied by value == null, then the above can be simplified to:

const maybeSkeleton = value => value ?? <Skeleton />;

<TableCell>{maybeSkeleton(value)}</TableCell>;

Or even:

<TableCell>{value ?? <Skeleton />}</TableCell>

[jared-dykstra]

I've built out a couple of sites over the past few months, both using react-loading-skeleton and the useSkeleton() hook (submitted in this PR) provides a Developer Experience that has been very well received. That's why I wanted to give back to this project.

Replying to @dvtng's comments:

• Strongly, strongly advise against "complecting" the concepts of value == null with isLoading. For example, react-loding-skeleton is useful for both loading states, and empty state. When loading completes, but there is no data, it is useful to leave the skeleton up, but disable animation and display an additional "no results found" message.

• Does it need to be a hook? No it doesn't. However, the developer experience is familiar.

[srmagura]

@dvtng raised some great points, but I agree with @jared-dykstra that useSkeleton offers a bit better DX than adding the const maybeSkeleton = value => loading ? <Skeleton /> : value; one-liner to many components.

I would like to merge this, but ideally we would first reach a consensus that this is a valuable feature.

[jared-dykstra]

@srmagura - I've squashed commits.

[dvtng]

I want to clarify my position: I'm not against this pattern, but I think it's just one possible pattern amongst many. I think the most suitable abstraction is dependent on the specifics of your data-fetching library / architecture and on specific UX decisions such as whether a skeleton should be shown when refetching stale data.

Treating value == null as loading is precisely the approach taken by the popular data-fetching library SWR, amongst others. When using these libraries, it would be much easier to write value ?? <Skeleton />. The concern @jared-dykstra raised regarding empty states can be solved by representing empty result sets as [] and not null. I'm not arguing that this approach is inherently better, but it is a popular one and does come with its unique advantages.

Other libraries, such as Apollo Client, expose a more explicit loading value. In these scenarios, you could adopt useSkeleton as proposed, but you might want to do something like this instead:

const { loading, error, data } = useQuery(QUERY);
const getValue = (selector) => loading ? <Skeleton /> : error ? <Error /> : selector(data);

<TableCell>{getValue((data) => data.deeply.nested.value)}</TableCell>;

This pattern addresses the fact that data may be null – either in error or loading states – and so to even retrieve value we need to first check these.

My point is to highlight how broad the design space is. Abstractions on top of react-loading-skeleton are useful, but I think they belong in user-space, not in the library.

[dvtng]

• Does it need to be a hook? No it doesn't. However, the developer experience is familiar.

As an aside, I strongly believe that hooks should be used as a last resort, because of all the limitations (rules of hooks) that they impose. You'd have a more composable primitive by rewriting useSkeleton as a regular function, that can be used along the lines of:

const withSkeleton = skeleton(loading);

<TableCell>{withSkeleton(value)}</TableCell>

[srmagura]

Really good points again. I think there is a middle ground here: don't add this functionality to the library, but instead, document useSkeleton / withSkeleton in the README as a recommended code pattern. This path is currently my preference.

If we go this route, we can simplify the useSkeleton code so that it is easier to understand:

• No need to use useMemo
• No need to return an object, just return the withSkeleton function directly

[jared-dykstra]

I think [abstractions] belong in user-space, not in the library.

If you do choose to add the useSkeleton() hook. (and I really hope you do!), it's very much an opt-in choice for developers. It doesn't detract from any other ways of interacting with this library.

Other libraries, such as Apollo Client, expose a more explicit loading value

Indeed. Here is how I have been using this hook in conjunction with Apollo

const { loading, error, data } = useQuery(QUERY);
const { withSkeleton } = useSkeleton(loading || error, {animate: !error});

<>
  {error && <Overlay>An error occurred</Overlay>}
  ...
  <TableCell>{withSkeleton(data?.deeply?.nested?.value)}</TableCell>;
  ...
<>

No need to return an object, just return the withSkeleton function directly

That's true. The only DX impact is const withSkeleton = useSkeleton() vs const { withSkeleton } = useSkeleton(). I just did it for consistency with other imports like useQuery above. I have not run a performance test comparing both.

[srmagura]

@jared-dykstra Do you want to make a new PR that updates the README to show the withSkeleton pattern? I think we could show it as both a hook and a one-liner.

[srmagura]

Closing since this has not seen activity in a while. I am sorry we decided not to accept your contribution @jared-dykstra. As I said, a PR that updates the README to show this code pattern would be much appreciated.

[jared-dykstra]

@jared-dykstra Do you want to make a new PR that updates the README to show the withSkeleton pattern? I think we could show it as both a hook and a one-liner.

I don't want to update docs without including corresponding code.

Thanks for closing this PR.