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(remix-react): Preserve search string in form action when action prop is omitted #3697

Merged
merged 15 commits into from Aug 10, 2022

Conversation

chaance
Copy link
Collaborator

@chaance chaance commented Jul 11, 2022

When a Form action is omitted, the default value is "." which resolves to the current route without search params. This is intended behavior when "." is passed to useResolvedPath, but Remix docs specify that when an action is omitted that the default action is the current location. Users would expect that to include the search string.

Closes: #927
Closes: #931

  • Docs
  • Tests

Testing Strategy:

Added tests to the existing form integration test file.

@changeset-bot
Copy link

changeset-bot bot commented Jul 11, 2022

🦋 Changeset detected

Latest commit: 9147639

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

This PR includes changesets to release 16 packages
Name Type
@remix-run/react Patch
create-remix Patch
remix Patch
@remix-run/dev Patch
@remix-run/eslint-config Patch
@remix-run/serve Patch
@remix-run/server-runtime Patch
@remix-run/cloudflare Patch
@remix-run/node Patch
@remix-run/deno Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/express Patch
@remix-run/netlify Patch
@remix-run/vercel Patch
@remix-run/architect 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

@chaance chaance changed the base branch from main to dev July 11, 2022 05:12
@MichaelDeBoey MichaelDeBoey changed the title fix: Preserve search string in form action when action prop is omitted fix(remix-react): Preserve search string in form action when action prop is omitted Jul 11, 2022
@ryanflorence
Copy link
Member

ryanflorence commented Jul 11, 2022

Remix docs specify that when an action is omitted that the default action is the current location

The docs are wrong then 🙈. If action is ommitted, the default action is "." which means the closest route in context, not the current location (just like <Link to=".">. Nested routing adds a new concept here (at the cost of some complexity).

Preserving search params seems like the right thing to do, but I'm trying to think about the symmetry here:

Default Browser/HTML Behavior

element preserves search
<a href=".">
<form method="get" action=".">
<form method="get">
✅ technically...

technically "yes", the form.action includes the search but the form fields wipe it out when submitted, so practically "no"

<form method="post" action=".">
<form method="post">

https://rv02u9.csb.app/foo/?bar#baz

So:

  • . means location.pathname only
  • omitted action means location.pathname + location.search + location.hash

What Form should do

The new concept with nested routes is that . means "closest route in context" instead of location.pathname.

The problem seems to be that we are defaulting action to ., which is inconsistent with browser behavior. We should default it to the closest route in context (the new concept) but then be consistent with the browser from there and use route.path + location.search + location.hash.

The changes on this PR treat . and an omitted action the same, which I don't think is what we want to do. Instead we should do this:

element preserves search
<Link to=".">
<Form method="get" action=".">
<Form method="get">
<form method="post" action=".">
<form method="post">

I expect the change to be pretty simple.

  • Make action optional, don't include a default "." anymore.
  • If there is no action, default to the resolvedRoute.path + location.search + location.hash

@chaance chaance merged commit 7d042b8 into dev Aug 10, 2022
@chaance chaance deleted the chance/use-form-action-fix branch August 10, 2022 20:35
mcansh pushed a commit that referenced this pull request Aug 11, 2022
… prop is omitted (#3697)

Co-authored-by: Ionut Botizan <ionut.botizan@haufe-lexware.com>
Co-authored-by: Javier Castro <javier.alejandro.castro@gmail.com>
@MichaelDeBoey MichaelDeBoey added the awaiting release This issue has been fixed and will be released soon label Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting release This issue has been fixed and will be released soon CLA Signed renderer:react
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: useSubmitImpl() not respects incoming query search when using POST
6 participants