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

feat: Improve Controller.getResponse() interface #1396

Merged
merged 1 commit into from Oct 17, 2021
Merged

Conversation

ntucker
Copy link
Collaborator

@ntucker ntucker commented Oct 17, 2021

BREAKING CHANGE: useDenormalized() return type changed
{
data: DenormalizeNullable<Shape['schema']>;
expiryStatus: ExpiryStatus;
expiresAt: number;
}

Motivation

Manifest expiry policy within getResponse() to make it more obvious what to do

Solution

Since the interface must changed for useDenormalized() and we want to get rid of it; mark as deprecated and make useStatefulResource() no longer depend on it.

Expiry behavior goal is https://resthooks.io/docs/getting-started/expiry-policy#expiry-status

However, triggering mechanisms are dependent on hook implementation, therefore we want to cede control of triggering the fetch and describe the behavior based on how that plays.

To describe we introduce

export enum ExpiryStatus {
  Invalid = 1,
  InvalidIfStale,
  Valid,
}

Valid

  • Will never suspend.
  • Might fetch if data is stale

InvalidIfStale

  • Will suspend if data is stale.
  • Might fetch if data is stale

Invalid

  • Will always suspend
  • Will always fetch

Alternatives

I did consider returning two methods:

isStale()  // determines if fetch should happen
isInvalid(maybePromise) // determines if suspense should happen; takes promise from fetch as argument

To correctly trigger fetches this would maintain referential equality until expiresAt or forceFetch change

This would change

  // If we are hard invalid we must fetch regardless of triggering or staleness
  const forceFetch = expiryStatus === ExpiryStatus.Invalid;

  const maybePromise = useMemo(() => {
    // null params mean don't do anything
    if ((Date.now() <= expiresAt && !forceFetch) || !key) return;
    // @ts-ignore
    return controller.fetch(endpoint, ...args);
    // 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]);

  // fully "valid" data will not suspend even if it is not fresh
  if (expiryStatus !== ExpiryStatus.Valid && maybePromise) {
    throw maybePromise;
  }

To

  const maybePromise = useMemo(() => {
    // null params mean don't do anything
    if (!expiryPolicy.isStale() || !key) return;
    // @ts-ignore
    return controller.fetch(endpoint, ...args);
    // we need to check against serialized params, since params can change frequently
    // eslint-disable-next-line react-hooks/exhaustive-deps
  }, [expiryPolicy, controller, key, state.lastReset]);

  // fully "valid" data will not suspend even if it is not fresh
  if (expiryPolicy.isInvalid(maybePromise)) {
    throw maybePromise;
  }

Follow up

In a subsequent PR, will add useSuspense:

/**
 * Ensure an endpoint is available.
 * Suspends until it is.
 *
 * `useSuspense` guarantees referential equality globally.
 * @see https://resthooks.io/docs/api/useSuspense
 * @throws {Promise} If data is not yet available.
 * @throws {NetworkError} If fetch fails.
 */
export default function useSuspense<
  E extends EndpointInterface<FetchFunction, Schema | undefined, undefined>,
  Args extends readonly [...Parameters<E>] | readonly [null],
>(
  endpoint: E,
  ...args: Args
): E['schema'] extends Exclude<Schema, null>
  ? Denormalize<E['schema']>
  : ReturnType<E> {
  const state = useContext(StateContext);
  const controller = useController();

  const key = args[0] !== null ? endpoint.key(...args) : '';
  const cacheResults = args[0] !== null && state.results[key];

  // Compute denormalized value
  const { data, expiryStatus, expiresAt } = useMemo(() => {
    // @ts-ignore
    return controller.getResponse(endpoint, ...args, state) as {
      data: Denormalize<E['schema']>;
      expiryStatus: ExpiryStatus;
      expiresAt: number;
    };
    // eslint-disable-next-line react-hooks/exhaustive-deps
  }, [
    cacheResults,
    state.indexes,
    state.entities,
    state.entityMeta,
    key,
    cacheResults,
  ]);

  // @ts-ignore
  const error = controller.getError(endpoint, ...args, state);

  // If we are hard invalid we must fetch regardless of triggering or staleness
  const forceFetch = expiryStatus === ExpiryStatus.Invalid;

  const maybePromise = useMemo(() => {
    // null params mean don't do anything
    if ((Date.now() <= expiresAt && !forceFetch) || !key) return;
    // @ts-ignore
    return controller.fetch(endpoint, ...args);
    // 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]);

  // fully "valid" data will not suspend even if it is not fresh
  if (expiryStatus !== ExpiryStatus.Valid && maybePromise) {
    throw maybePromise;
  }

  if (error) throw error;

  return data;
}

@ntucker ntucker force-pushed the getresponse branch 6 times, most recently from fa94b31 to 8ad8485 Compare October 17, 2021 03:45
@codecov-commenter
Copy link

codecov-commenter commented Oct 17, 2021

Codecov Report

Merging #1396 (16bf3de) into master (2ac471d) will decrease coverage by 0.09%.
The diff coverage is 92.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1396      +/-   ##
==========================================
- Coverage   98.61%   98.51%   -0.10%     
==========================================
  Files          92       93       +1     
  Lines        1728     1747      +19     
  Branches      345      346       +1     
==========================================
+ Hits         1704     1721      +17     
- Misses          5        6       +1     
- Partials       19       20       +1     
Impacted Files Coverage Δ
packages/core/src/endpoint/adapter.ts 88.88% <50.00%> (+1.38%) ⬆️
packages/legacy/src/shapeToEndpoint.ts 66.66% <66.66%> (ø)
packages/core/src/controller/Controller.ts 93.90% <100.00%> (-0.62%) ⬇️
packages/core/src/controller/Expiry.ts 100.00% <100.00%> (ø)
packages/core/src/index.ts 100.00% <100.00%> (ø)
...kages/core/src/react-integration/hooks/useCache.ts 100.00% <100.00%> (ø)
...es/core/src/react-integration/hooks/useResource.ts 100.00% <100.00%> (+2.43%) ⬆️
...ckages/core/src/state/selectors/useDenormalized.ts 100.00% <100.00%> (ø)
packages/legacy/src/index.ts 100.00% <100.00%> (ø)
packages/legacy/src/useStatefulResource.ts 100.00% <100.00%> (+6.25%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ac471d...16bf3de. Read the comment docs.

@ntucker ntucker force-pushed the getresponse branch 2 times, most recently from daf40ad to ae062b6 Compare October 17, 2021 04:38
BREAKING CHANGE: useDenormalized() return type changed
{
  data: DenormalizeNullable<Shape['schema']>;
  expiryStatus: ExpiryStatus;
  expiresAt: number;
}
@ntucker ntucker merged commit f57a909 into master Oct 17, 2021
@ntucker ntucker deleted the getresponse branch October 17, 2021 05:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants