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 fetcherKey APIs and update fetcher persistence architecture #10949

Closed
wants to merge 18 commits into from

Conversation

brophdawg11
Copy link
Contributor

@brophdawg11 brophdawg11 commented Oct 19, 2023

Implements the updated version of remix-run/remix#7698

Supersedes #10945

Todo:

  • Should we remove non-idle fetchers from router.state entirely? Right now they remain in there when they return to idle for one subscribe call so we can hand-off the data to the React layer. However, would it make sense to remove them before calling subscribers and pass the data in a subsequent argument?
    subscriber(state, { fetcherData, unstable_viewTransitionOpts })
  • Look into cleanup behavior for fetchers that return data but never have a useFetcher register to grab it so they never get a ref count
  • Expose fetcher key on useFetchers

@changeset-bot
Copy link

changeset-bot bot commented Oct 19, 2023

🦋 Changeset detected

Latest commit: 65d6b8c

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 Minor
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

@brophdawg11 brophdawg11 changed the title Add fetcher key APIs and persistence Add fetcherKey APIs and update fetcher persistence architecture Oct 20, 2023
Comment on lines 354 to 358
type FetchersContextObject = {
data: Map<string, any>;
register: (key: string) => void;
unregister: (key: string) => 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.

This context holds the fetcher data handed off from the router and exposes it for useFetcher

Comment on lines +477 to +481
newState.fetchers.forEach((fetcher, key) => {
if (fetcher.data !== undefined) {
fetcherData.current.set(key, fetcher.data);
}
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One-time fetcher data handoff. As soon as a router fetcher returns to idle, it comes through here one time and then is deleted inside the router. We stick the data into our fetcherData Map and clean it up when fetcherRefs returns to zero for a given key.

Need to look into cleanup behavior for fetchers that return data but never have a useFetcher register to grab it...

fromRouteId: currentRouteId,
unstable_viewTransition: options.unstable_viewTransition,
});
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aaand we're back to one useSubmit 🤣 .

@@ -1504,23 +1563,10 @@ export function useFormAction(
return createPath(path);
}

function createFetcherForm(fetcherKey: string, routeId: string) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Inlined in useFetcher

Comment on lines +1588 to +1591
let [fetcherKey, setFetcherKey] = React.useState<string>(key || "");
if (!fetcherKey) {
setFetcherKey(getUniqueFetcherId());
}
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 should be the right way to assign the fetcher keys, we had some weird behaviors with the old approach of incrementing in a state initializer.

Comment on lines +1602 to +1606
// Register/deregister with FetchersContext
React.useEffect(() => {
register(fetcherKey);
return () => unregister(fetcherKey);
}, [fetcherKey, register, unregister]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Register and unregister so we can ref count fetchers for cleanup

@brophdawg11
Copy link
Contributor Author

@brophdawg11 brophdawg11 deleted the brophdawg11/fetcher-persist branch October 26, 2023 19:46
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