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

Updates for v7_fetcherPersist post-processing logic #10977

Merged
merged 2 commits into from
Oct 30, 2023

Conversation

brophdawg11
Copy link
Contributor

@brophdawg11 brophdawg11 commented Oct 30, 2023

When v7_fetcherPersist is enabled we open up some new code flows for fetcher result processing that we need to consider.

  • Previously, when fetchers were aborted on unmount, we would end up ignoring all results from those unmounted fetchers via if (request.signal.aborted)
  • Now that we don't abort the fetchers immediately (when the flag is enabled) we need to determine if we should process the fetcher result or not

We do this by lifting the ref counting up into the router so it knows if any instances of a given fetcher key are still mounted in the UI.

  • If no instances are mounted at the time the fetcher completes, we don't process the result - like same as before when we aborted on unmount
  • If some instances are still mounted (meaning some other new component picked up the fetcher via useFetcher({ key }), then we do process the result

@changeset-bot
Copy link

changeset-bot bot commented Oct 30, 2023

🦋 Changeset detected

Latest commit: 7a29035

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

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

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


const FetchersContext = React.createContext<FetchersContextObject | null>(null);
const FetchersContext = React.createContext<FetchersContextObject>(new Map());
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 ref counting now lives in the router so this fetchers context just holds the data

) => {
deletedFetchers.forEach((key) => fetcherData.current.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.

The router tells us which fetchers can be deleted (once their ref counts go to 0)

Comment on lines -1250 to -1273
let registerFetcher = React.useCallback(
(key: string) => {
let count = fetcherRefs.current.get(key);
if (count == null) {
fetcherRefs.current.set(key, 1);
} else {
fetcherRefs.current.set(key, count + 1);
}
},
[fetcherRefs]
);

let unregisterFetcher = React.useCallback(
(key: string) => {
let count = fetcherRefs.current.get(key);
if (count == null || count <= 1) {
fetcherRefs.current.delete(key);
fetcherData.current.delete(key);
} else {
fetcherRefs.current.set(key, count - 1);
}
},
[fetcherData, fetcherRefs]
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer ref counting via the context

@brophdawg11 brophdawg11 merged commit 1500288 into release-next Oct 30, 2023
3 checks passed
@brophdawg11 brophdawg11 deleted the brophdawg11/fetcher-post-processing branch October 30, 2023 20:33
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 6.18.0-pre.1 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Copy link
Contributor

🤖 Hello there,

We just published version 6.18.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

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.

None yet

1 participant