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

Fix issues with useFormAction/useResolvedPath for dot paths in param/splat routes #10983

Merged
merged 8 commits into from
Nov 3, 2023

Conversation

brophdawg11
Copy link
Contributor

Second shot at a fix for #10922

Originally fixed in #10933 and reverted in #10965 due to issues.

The main issue seems to boil down to useResolvedPath's longstanding behavior of using pathnameBase to construct the current route hierarchy. pathnameBase never contains the splat - but pathname does.

This breaks a few existing uni tests that rely on the existing behavior - need to review with @mjackson.

Copy link

changeset-bot bot commented Oct 31, 2023

🦋 Changeset detected

Latest commit: 22a3657

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

This PR includes changesets to release 4 packages
Name Type
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

@brophdawg11
Copy link
Contributor Author

Looking at some of the existing tests, I can see the dropped splat maybe being expected for a use case such as:

// Assume current url is /parent/something
<Route path="parent">
  <Route path="static" element={<h1>Static</h1>} />
  <Route path="*" element={<Link to="./static">Click</Link>} />
</Route>

Where you want the link to go to /parent/static and not /parent/something/static - but that's somewhat different from the current behavior in non-splat routes:

// Assume current url is /parent/something
<Route path="parent">
  <Route path="static" element={<h1>Static</h1>} />
  <Route path=":param" element={<Link to="./static">Click</Link>} />
</Route>

When using a dynamic param that link will resolve to /parent/something/static

@brophdawg11 brophdawg11 linked an issue Oct 31, 2023 that may be closed by this pull request
@brophdawg11
Copy link
Contributor Author

@mjackson and I traced this back to #8086 and determined its just been a long-standing bug for splat route handling. The example there shows a .. going up to /files which implies that a . on a splat route should contain the splat portion.

@@ -2895,7 +2973,7 @@ function testDomRouter(
let { container } = render(<RouterProvider router={router} />);

expect(container.querySelector("form")?.getAttribute("action")).toBe(
"/foo?a=1"
"/foo/bar?a=1"
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 existing test was asserting the buggy behavior. When no action is specified it should inherit the current path including the splat portion

@@ -2915,7 +2993,7 @@ function testDomRouter(
let { container } = render(<RouterProvider router={router} />);

expect(container.querySelector("form")?.getAttribute("action")).toBe(
"/foo"
"/foo/bar"
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 existing test was asserting the buggy behavior. When action="." is specified it should inherit the current path including the splat portion

@@ -530,7 +530,7 @@ describe("<Link> href", () => {
});

expect(renderer.root.findByType("a").props.href).toEqual(
"/inbox/messages"
"/inbox/messages/abc"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above - this asserted the buggy behavior

expect(container.querySelector("form")?.getAttribute("action")).toBe(
"/foo"
);
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New tests we didn't have before to ensure that the useResolvedPath changes don't make us accidentally pick up child route portions

Copy link
Member

@mjackson mjackson left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@brophdawg11 brophdawg11 merged commit fe066bd into dev Nov 3, 2023
3 checks passed
@brophdawg11 brophdawg11 deleted the brophdawg11/fix-form-action branch November 3, 2023 14:41
Copy link
Contributor

🤖 Hello there,

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

Copy link
Contributor

🤖 Hello there,

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

jonathanpruvost pushed a commit to concordnow/react-router that referenced this pull request Nov 28, 2023
jonathanpruvost pushed a commit to concordnow/react-router that referenced this pull request Nov 28, 2023
brophdawg11 added a commit that referenced this pull request Dec 1, 2023
brophdawg11 added a commit that referenced this pull request Dec 1, 2023
brophdawg11 added a commit that referenced this pull request Dec 1, 2023
* Revert "Fix other code paths for resolveTo from a splat route (#11045)"

This reverts commit 58d421f.

* Revert "Add additional test case for #10983"

This reverts commit f320378.

* Revert "Fix issues with useFormAction/useResolvedPath for dot paths in param/splat routes (#10983)"

This reverts commit fe066bd.

* Fix test

* Add changeset
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.

[Bug]: useResolvedPath breaks <Form> when used in a splat route
2 participants