Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Controller.fetchIfStale() #2053

Closed
ntucker opened this issue Jun 17, 2022 · 6 comments
Closed

RFC: Controller.fetchIfStale() #2053

ntucker opened this issue Jun 17, 2022 · 6 comments
Labels
enhancement New feature or request

Comments

@ntucker
Copy link
Collaborator

ntucker commented Jun 17, 2022

Motivation

  • fetch-as-render pattern results in over-fetching (non-stale data is fetched on every transition)
  • centralize more handling - less in hooks = happier world

Current world

Currently final ‘stale’ logic exists in hooks

const { data, expiryStatus, expiresAt } = controller.getResponse(endpoint, ...args, state);
const forceFetch = expiryStatus === ExpiryStatus.Invalid
const maybePromise = useMemo(() => {
    // null params mean don't do anything
    if ((Date.now() <= expiresAt && !forceFetch) || !key) return;

    return controller.fetch(endpoint, ...(args as readonly [...Parameters<E>]));
    // we need to check against serialized params, since params can change frequently
    // eslint-disable-next-line react-hooks/exhaustive-deps
  }, [expiresAt, controller, key, forceFetch, state.lastReset]);

This isn’t too bad; but it does mean some logic is repeated in any implementation.

The non-lifecycle based logic can be extracted as such:

Date.now() <= expiresAt && expiryStatus !== ExpiryStatus.Invalid

Proposal

💡 Need feedback on name. I don’t like how long fetchIfStale is!

Add a new controller dispatch that will only fetch if the result is stale. If it ends up not fetching the promise will immediately resolve.

  • Only accepts sideEffect: false endpoints

Open questions

  • what should it be called
  • what should it resolve to as it might not fetch but that means the data already exists?
    • maybe denormalized form and waits until commit.
    • or perhaps the resolve value lets you know if it fetched at all
  • always returning a promise means it is unusable in hooks pre-react18. can we possibly hook up a callback in the middleware that determines whether it fetches non-async?

Dispatch resolution control

dispatch adds to action a callback. if this callback is called it returns the dispatch early.

Instead of

return next(action);
return Whatever;

And you can still continue processing the request by doing the rest of it async

// we don't wait on resolution
next(action);
return Whatever;

Doing this would also enable not having to send resolve/reject in fetch meta; but just take dispatch return value. This would be actual promise used by the NetworkManager so referential equality checks could be performed against it.

This means dispatch() now has a variable return type based on what the middlewares do. This could have weird implications for user-defined managers; so perhaps we should add type inference based on middlewares? This can be delayed tho.

@ntucker ntucker added the enhancement New feature or request label Jun 17, 2022
@ntucker
Copy link
Collaborator Author

ntucker commented Jun 17, 2022

Council notes

  • Name is actually quite good
  • Migrate by doing middleware change first, then add new dispatcher
  • Still open question about best return value given this is an important use case with no access to state:

Anansi router example:

  resolveData: async (controller: Controller, match: { id: string }) => {
    if (match) {
      // don't block on comments but start fetching
      controller.fetchIfStale(CommentResource.getList, { postId: match.id });
      const post = await controller.fetchIfStale(PostResource.get, {
        id: match.id,
      });
      await Promise.allSettled([
        controller.fetchIfStale(
          UserResource.get,
          post.userId ? { id: post.userId } : (null as any),
        ),
        controller.fetchIfStale(getImage, {
          src: UserResource.fromJS({ id: post.userId }).profileImage,
        }),
      ]);
    }

@gregor-mueller
Copy link

This would be really helpful for us!

Name suggestion: Controller.provide as this will provide the data (from cache if fresh, from API if stale).

@ntucker
Copy link
Collaborator Author

ntucker commented Oct 20, 2022

Thanks for the input @gregor-mueller ! What do you expect/want for the return value to be? The resolution of the promise, or the processed data from the cache?

@gregor-mueller
Copy link

gregor-mueller commented Oct 21, 2022

Thanks for the quick response! I'd expect the processed data from the cache.

@ntucker
Copy link
Collaborator Author

ntucker commented Nov 20, 2022

@gregor-mueller In case you just want the state, you can use https://resthooks.io/docs/api/Controller#getState

The rest of the functionality will be soon added

@ntucker
Copy link
Collaborator Author

ntucker commented Jan 31, 2023

Closing in favor of discussion: #2402

@ntucker ntucker closed this as completed Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants