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

Add support for fetcher key specification and persistence #10945

Closed
wants to merge 9 commits into from

Conversation

brophdawg11
Copy link
Contributor

Implementation for remix-run/remix#7698

  • Users can provide their own fetcher keys via useFetcher({ key: "abc" }), allowing you to access the same fetcher instance from hook invocations in separate parts of your app. Current internal keys are incrementing values ("1", "2", etc.) so they've been updated with a __ prefix to avoid potential collisions with user-specific values ("__1", "__2", etc.).
    • Maybe it's time to turn these keys into symbols?
  • Fetcher persistence is available for submitting fetchers only. useFetcher({ persist: true }) will cause the fetcher to persist beyond it's component unmount if and only if it was non-idle at the time of unmount. It will delay cleanup until it returns to an idle state.

@changeset-bot
Copy link

changeset-bot bot commented Oct 18, 2023

🦋 Changeset detected

Latest commit: f4fa0c1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
react-router-dom Minor
@remix-run/router Patch
react-router Minor
react-router-dom-v5-compat Minor
react-router-native Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Comment on lines -4678 to +4679
// Resolve Comp2 loader and complete navigation - Comp1 fetcher is still
// reflected here since deleteFetcher doesn't updateState
// TODO: Is this expected?
// Resolve Comp2 loader and complete navigation
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had a small bug before where a deleteFetcher wouldn't trigger a top-down state.fetchers update so useFetchers could be slightly delayed in showing the removal until the next top-down update

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the new deleteFetcherAndUpdateState for the bug fix

Comment on lines 1546 to 1556
let [fetcherKey] = React.useState(() => String(++fetcherId));
let [_fetcherKey] = React.useState(() => `__${String(++fetcherId)}`);
let fetcherKey = key ? key : _fetcherKey;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer user-specified key if available. Also prefixed our current internal keys with __ to avoid collisions with user-specified IDs.

getFetcher<TData = any>(key?: string): Fetcher<TData>;
getFetcher<TData = any>(key: string): Fetcher<TData>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a type definition bug - key is not optional in the implementations of getFetcher/deleteFetcher

type SubmitFetchOptions = BaseNavigateOrFetchOptions &
BaseSubmissionOptions & {
persist?: boolean;
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

persist can only be specified on a submisssion fetch

@@ -1442,9 +1443,10 @@ function useSubmitFetcher(
body,
formMethod: options.method || (method as HTMLFormMethod),
formEncType: options.encType || (encType as FormEncType),
persist,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The useFetcher({ persist }) value is proxied through to the router at router.fetch time, not at useFetcher-creation time. This makes it easier to target submitting fetchers only

handleFetcherAction(key, routeId, path, match, matches, submission);
return;
}

// Store off the match so we can call it's shouldRevalidate on subsequent
// revalidations
fetchLoadMatches.set(key, { routeId, path });
persistedFetchers.delete(key);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fetcher.load removes any prior fetcher.submit persistence for that key

...(didAbortFetchLoads || revalidatingFetchers.length > 0
? { fetchers: new Map(state.fetchers) }
: {}),
fetchers: new Map(state.fetchers),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was an error-prone optimization from the original implementation. Since we're inside handleFetcherAction we should always update the stateful fetchers Map (like we do in the navigation interruption case a few lines above).

@@ -2362,8 +2384,7 @@ export function createRouter(init: RouterInit): Router {
});
}

function deleteFetcher(key: string): void {
let fetcher = state.fetchers.get(key);
function cleanupFetcher(key: string, fetcher: Fetcher | undefined): void {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extract cleanupFetchers as a way to clean them up without touching state (since we need this inside updateState calls for the delayed cleanup)

@@ -2613,7 +2654,7 @@ export function createRouter(init: RouterInit): Router {
createHref: (to: To) => init.history.createHref(to),
encodeLocation: (to: To) => init.history.encodeLocation(to),
getFetcher,
deleteFetcher,
deleteFetcher: deleteFetcherAndUpdateState,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brophdawg11
Copy link
Contributor Author

"Pivot" scene from Friends

All the comments in here are mostly out of date due to the changes we made to remix-run/remix#7698, going to close this in favor of a fresh PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant