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): detect index route from manifest in useFormAction #3127

Closed

Conversation

penspinner
Copy link
Contributor

The useFormAction hook detects the file ID ending in "/index" to determine whether or not to return a form action with the ?index search param. This holds true in Remix's default route convention. However, in the case where the consumer defines custom routes, this may not hold true anymore.


Testing Strategy:

I'm using the Remix Flat Routes flat-folders convention in my project.

Let's say I have a route _auth.dashboard.index/_routes.tsx. That's considered to be an index route in the flat-folders convention. However, rendering a <Form> in that route results in a form with the action "/dashboard" while the action should be "/dashboard?index".

@penspinner penspinner marked this pull request as ready for review May 7, 2022 23:06
@penspinner penspinner force-pushed the fix-use-form-action-index branch from e54a414 to 874e24c Compare May 8, 2022 15:15
@MichaelDeBoey MichaelDeBoey changed the title fix(useFormAction): an index route should be detected from the manifest rather than the file id fix(remix-react): detect index route from manifest in useFormAction May 8, 2022
packages/remix-react/components.tsx Outdated Show resolved Hide resolved
@MichaelDeBoey
Copy link
Member

@penspinner Could you please also add a test for this change?

@MichaelDeBoey MichaelDeBoey added needs-response We need a response from the original author about this issue/PR renderer:react labels May 8, 2022
@penspinner penspinner requested a review from MichaelDeBoey May 8, 2022 21:21
@penspinner penspinner force-pushed the fix-use-form-action-index branch 2 times, most recently from ad1aa99 to 8cca2aa Compare May 9, 2022 01:05
integration/form-test.ts Outdated Show resolved Hide resolved
integration/form-test.ts Outdated Show resolved Hide resolved
integration/form-test.ts Outdated Show resolved Hide resolved
integration/form-test.ts Outdated Show resolved Hide resolved
@penspinner penspinner force-pushed the fix-use-form-action-index branch from 8cca2aa to ca58dcc Compare May 9, 2022 01:10
@penspinner penspinner requested a review from MichaelDeBoey May 9, 2022 01:19
@MichaelDeBoey MichaelDeBoey removed the needs-response We need a response from the original author about this issue/PR label May 9, 2022
@changeset-bot
Copy link

changeset-bot bot commented Nov 16, 2022

⚠️ No Changeset found

Latest commit: cec2d4a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@kiliman
Copy link
Collaborator

kiliman commented Nov 16, 2022

Awesome work @penspinner. BTW: I did fix remix flat routes to work around this issue. I append /index to all the index route ids so Remix works properly. Once this PR lands, I'll be able to remove the hack. Thanks!

@MichaelDeBoey
Copy link
Member

Hi @penspinner!

Are you still interested in getting this one merged?

If so, please rebase onto latest dev & resolve conflicts

@MichaelDeBoey MichaelDeBoey added the needs-response We need a response from the original author about this issue/PR label May 1, 2023
@penspinner
Copy link
Contributor Author

@MichaelDeBoey This seems to have been fixed in React Router. Gonna close this.

@penspinner penspinner closed this May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed needs-response We need a response from the original author about this issue/PR renderer:react
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants