Add support for fetcher key specification and persistence #10945
Conversation
🦋 Changeset detectedLatest commit: f4fa0c1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
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 |
| // 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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
See the new deleteFetcherAndUpdateState for the bug fix
packages/react-router-dom/index.tsx
Outdated
| let [fetcherKey] = React.useState(() => String(++fetcherId)); | ||
| let [_fetcherKey] = React.useState(() => `__${String(++fetcherId)}`); | ||
| let fetcherKey = key ? key : _fetcherKey; |
There was a problem hiding this comment.
Prefer user-specified key if available. Also prefixed our current internal keys with __ to avoid collisions with user-specified IDs.
packages/router/router.ts
Outdated
| getFetcher<TData = any>(key?: string): Fetcher<TData>; | ||
| getFetcher<TData = any>(key: string): Fetcher<TData>; |
There was a problem hiding this comment.
This is a type definition bug - key is not optional in the implementations of getFetcher/deleteFetcher
packages/router/router.ts
Outdated
| type SubmitFetchOptions = BaseNavigateOrFetchOptions & | ||
| BaseSubmissionOptions & { | ||
| persist?: boolean; | ||
| }; |
There was a problem hiding this comment.
persist can only be specified on a submisssion fetch
packages/react-router-dom/index.tsx
Outdated
| @@ -1442,9 +1443,10 @@ function useSubmitFetcher( | |||
| body, | |||
| formMethod: options.method || (method as HTMLFormMethod), | |||
| formEncType: options.encType || (encType as FormEncType), | |||
| persist, | |||
There was a problem hiding this comment.
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
packages/router/router.ts
Outdated
| 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); |
There was a problem hiding this comment.
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), |
There was a problem hiding this comment.
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).
packages/router/router.ts
Outdated
| @@ -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 { | |||
There was a problem hiding this comment.
Extract cleanupFetchers as a way to clean them up without touching state (since we need this inside updateState calls for the delayed cleanup)
packages/router/router.ts
Outdated
| @@ -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, | |||
There was a problem hiding this comment.
Fixes the bug mentioned in https://github.com/remix-run/react-router/pull/10945/files#r1364167077
|
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. |
Implementation for remix-run/remix#7698
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.).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.