atlassian-labs / react-resource-router

Configuration driven routing solution for React SPAs that manages route matching, data fetching and progressive rendering
https://atlassian-labs.github.io/react-resource-router
Apache License 2.0
202 stars 28 forks source link

Do not expose a data key until loading has finished and there is no error to make more restful. #112

Closed slevens-atlas closed 2 years ago

slevens-atlas commented 2 years ago

The current implementation of RouteResourceResponseBase includes a data key which will always be null as defined in https://github.com/atlassian-labs/react-resource-router/blob/a4e0f0358c9987f5b0398ac76deaf491a96ce308/src/controllers/resource-store/constants.ts#L8

I suggest we remove the key entirely and let it only be set by the response successfully returning data.

This will have the flow on effect of improving the typescript checking and will force developers to check loading and error before the data key will even appear.

Like this:

export const Home = () => {
  const homeResponse = useResource(homeResource);

  if (homeResponse.error) {
    return <Error error={home.error} />;
  }

  if (homeResponse.loading) {
    return <Loading />;
  }

  // before this there is no data key on homeResponse
  return <div>{homeResponse.data.home.content}</div>;
};
slevens-atlas commented 2 years ago

I have a branch with the changes ready if someone can give me contributor access I can open a PR and attach to review.

theKashey commented 2 years ago

You are missing the "stale while invalidate" model, when you can have old data while loading a new one. However it will be very useful to know a single state of a given resource: loading, stale, error, arrived(🤷‍♂️).

slevens-atlas commented 2 years ago

This makes a lot of sense, maybe we could include a state key and expose keys based on that? Could potentially be too rigid though. Otherwise if we are happy with the current implementation I can close this issue.

slevens-atlas commented 2 years ago

I'm going to close this issue. I think it is fine in its current state and leave it up to the dev to determine how they want to handle it