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

Stabilize useNavigate/useSubmit/fetcher.submit for data routers #10336

Merged
merged 22 commits into from
Apr 14, 2023

Conversation

brophdawg11
Copy link
Contributor

@brophdawg11 brophdawg11 commented Apr 12, 2023

This work closes a handful of things:

@changeset-bot
Copy link

changeset-bot bot commented Apr 12, 2023

🦋 Changeset detected

Latest commit: 9f164a6

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 linked an issue Apr 12, 2023 that may be closed by this pull request
Comment on lines +182 to +190
if (options.action) {
action = options.action;
} else {
// When grabbing the action from the element, it will have had the basename
// prefixed to ensure non-JS scenarios work, so strip it since we'll
// re-prefix in the router
let attr = target.getAttribute("action");
action = attr ? stripBasename(attr, basename) : null;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A few things going on here:

  • We now fallback to action:null if nothing is available, which lets the router handle differing behavior for a missing action on submission navigations via <Form>
  • We want our <form action>/<button formaction> values to contain the basename if it exists for non-JS, but since the router will handle prepending the basename now we strip it back off on our resolved action
  • If a user if providing an action in options it's not expected to have a basename (same as <Form action>)

future: opts?.future,
future: {
...opts?.future,
v7_prependBasename: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Always turn this on for RR - it used to be RR doing the prepending, now it's the router

let { router } = useDataRouterContext(DataRouterHook.UseSubmitImpl);
let defaultAction = useFormAction();
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 what causes fetcher.submit to be unstable, since it depends on useLocation. Now we only depend on the current routeId and we let the router do the relative path resolving

@@ -344,4 +347,1343 @@ describe("useNavigate", () => {
`);
});
});

describe("when relative navigation is handled via React Context", () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These mimic the tests in <Navigate> and we have a set of them for both <MemoryRouter> and <RouterProvider> using useNavigate. Then we test that useNavigate in <MemoryRouter> is unstable while it is stable in <RouterProvider>

@@ -254,15 +256,18 @@ export function createMemoryRouter(
routes: RouteObject[],
opts?: {
basename?: string;
future?: FutureConfig;
future?: Partial<Omit<FutureConfig, "v7_prependBasename">>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

createMemoryRouter does not accept v7_prependBasename since it always wants to prepend

let isDataRouter = React.useContext(DataRouterContext) != null;
// Conditional usage is OK here because the usage of a data router is static
// eslint-disable-next-line react-hooks/rules-of-hooks
return isDataRouter ? useNavigateStable() : useNavigateUnstable();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fork to the stable version if using a data router

@@ -3034,20 +3055,111 @@ function isSubmissionNavigation(
return opts != null && "formData" in opts;
}

function normalizeTo(
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 basically a combination of useResolvedPath and useFormAction

@brophdawg11 brophdawg11 linked an issue Apr 12, 2023 that may be closed by this pull request
@brophdawg11 brophdawg11 linked an issue Apr 12, 2023 that may be closed by this pull request
@brophdawg11 brophdawg11 marked this pull request as ready for review April 12, 2023 21:00
Comment on lines -1050 to +1064
let { path, submission, error } = normalizeNavigateOptions(
let normalizedPath = normalizeTo(
state.location,
state.matches,
basename,
future.v7_prependBasename,
to,
future,
opts?.fromRouteId,
opts?.relative
);
let { path, submission, error } = normalizeNavigateOptions(
future.v7_normalizeFormMethod,
false,
normalizedPath,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Normalizing the path (normalizeTo) is now separate from normalizing the submission info (normalizeNavigateOptions).

@brophdawg11
Copy link
Contributor Author

✅ Reviewed over discord share with @jacob-ebey

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 6.11.0-pre.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!

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 6.11.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!

julienw added a commit to julienw/react-router that referenced this pull request Feb 13, 2024
…ubmit is stable across reloads

This was implemented originally in remix-run#10336.
julienw added a commit to julienw/react-router that referenced this pull request Feb 13, 2024
…ubmit is stable across reloads

This was implemented originally in remix-run#10336.
julienw added a commit to julienw/react-router that referenced this pull request Feb 13, 2024
…ubmit is stable across rerenders

This was implemented originally in remix-run#10336.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants