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

Form submitting to incorrect routes #14

Closed
nakleiderer opened this issue Jun 7, 2022 · 14 comments
Closed

Form submitting to incorrect routes #14

nakleiderer opened this issue Jun 7, 2022 · 14 comments

Comments

@nakleiderer
Copy link

After migrating to this library, my forms are submitting to incorrect routes.

Here's a repro (the README has a lot more detail): https://stackblitz.com/edit/node-kh9dmu?file=README.md

@puchesjr
Copy link

puchesjr commented Nov 3, 2022

We have an issue, but it is related to a userFetcher.Form in our layout route. The useFetcher.Form is in our search.tsx sidebar but calls the Action on the search._index.tsx perhaps we need to declare which Action the useFetcher should call?

@kiliman
Copy link
Owner

kiliman commented Nov 3, 2022

I recommend being explicit with your action. And remember, if you're posting to an index route, you should always append ?index to the path.

@shadyendless
Copy link

shadyendless commented Nov 4, 2022

After migrating to this library, my forms are submitting to incorrect routes.

Here's a repro (the README has a lot more detail): https://stackblitz.com/edit/node-kh9dmu?file=README.md

Looking at your Stackblitz, it looks as though the actions are being invoked properly. I think I may be misunderstanding the issue. Could you explain in more detail how each action is supposed to function?

I've attached a video of the behavior I am seeing.

CleanShot.2022-11-03.at.21.17.23.mp4

EDIT:: Disregard this, I tweaked the example you had a bit and was able to create a reproduction of the issue. Can find a modified Stackblitz here: https://stackblitz.com/edit/node-7u4xmy?file=app/flat/_site._index.tsx

@kentcdodds
Copy link

I'm also experiencing this issue, except with a <Form> rather than a fetcher or submit.

One thing I noticed is that when running npx remix routes with flat routes, all the routes are prefixed with / whereas with the default all routes do not have a / prefix. I tried updating things manually in node_modules and removing that prefix didn't affect the outcome so probably not related.

I'll put together a reproduction for you.

@kentcdodds
Copy link

kentcdodds commented Nov 4, 2022

Here's the npx remix routes for the default route config (the one that works):

<Routes>
  <Route file="root.tsx">
    <Route path="ships/:shipId" index file="routes/ships.$shipId/index.tsx" />
    <Route index file="routes/index.tsx" />
  </Route>
</Routes>

And here's the npx remix routes for the flat route config (the one that does not work):

<Routes>
  <Route file="root.tsx">
    <Route index file="routes/index.tsx" />
    <Route path="/ships/:shipId" index file="routes/ships_.$shipId._index.tsx" />
  </Route>
</Routes>

NOTE: I noticed that StackBlitz isn't completely reliable in running things correctly so I suggest downloading and running locally.

Also one thing I noticed is the form that's actually rendered differs in the two examples. In the one that works, it includes ?index at the end of the action attribute, and the flat routes one does not.

I should also mention that even if I disable JavaScript and add the ?index manually, the form does not get routed to the correct route anyway.

@kiliman
Copy link
Owner

kiliman commented Nov 5, 2022

Interesting. Thanks for the repro. I'm finishing up my Remix Conf presentation, but I will get to this ASAP.

@lukebowerman
Copy link

I ran into this same issue when upgrading and found that the breakage is specifically tied to upgrading from Remix 1.7.3 to 1.7.4. You can it with a fork of Kent's examples above:

(Kent's original examples used ^1.7.2. which resolved to 1.7.5)

It's not immediately clear to me what change in 1.7.4 caused the issue but I'm suspicious of: remix-run/remix#4376 (deals with index matching so perhaps flat-routes is interacting with this unexpectedly?)

@kiliman
Copy link
Owner

kiliman commented Nov 8, 2022

Thanks for the research. I was surprised when this started breaking, as I haven't had any issues in my app which has over 100 routes, but I also haven't updated to the latest yet either. Anyway, I will get to the bottom of this. Remix flat routes should generate the same routing structure as the conventional routes.

@kiliman
Copy link
Owner

kiliman commented Nov 9, 2022

I found the problems. First is a bug in my route generation, but the other is a bug in Remix in how it matches the index route. It was checking if the URL ends in /index (which won't be true for flat routes), when it should be looking at route.index === true since that's already defined in the route config.

Here's the patch for Remix. I'll be updating the flat routes package.

diff --git a/node_modules/@remix-run/server-runtime/dist/server.js b/node_modules/@remix-run/server-runtime/dist/server.js
index 5ac2304..58e39f3 100644
--- a/node_modules/@remix-run/server-runtime/dist/server.js
+++ b/node_modules/@remix-run/server-runtime/dist/server.js
@@ -503,7 +503,7 @@ function isIndexRequestUrl(url) {
 function getRequestMatch(url, matches) {
   let match = matches.slice(-1)[0];
 
-  if (isIndexRequestUrl(url) && match.route.id.endsWith("/index")) {
+  if (isIndexRequestUrl(url) && match.route.index) {
     return match;
   }

@kiliman
Copy link
Owner

kiliman commented Nov 9, 2022

One other Remix patch. This time it's to fix the index handling for the Form action. Again Remix is checking for ending in /index, when it should be getting the index prop from the routes config (available from entry context manifest)

diff --git a/node_modules/@remix-run/react/dist/esm/components.js b/node_modules/@remix-run/react/dist/esm/components.js
index e884e7b..56ddff4 100644
--- a/node_modules/@remix-run/react/dist/esm/components.js
+++ b/node_modules/@remix-run/react/dist/esm/components.js
@@ -746,6 +746,7 @@ FormImpl.displayName = "FormImpl";
  */
 function useFormAction(action, // TODO: Remove method param in v2 as it's no longer needed and is a breaking change
 method = "get") {
+  let { manifest }= useRemixEntryContext()
   let {
     id
   } = useRemixRouteContext();
@@ -760,7 +761,7 @@ method = "get") {
     search,
     hash
   } = resolvedPath;
-  let isIndexRoute = id.endsWith("/index");
+  let isIndexRoute = manifest.routes[id].index
 
   if (action == null) {
     search = location.search;

@kiliman
Copy link
Owner

kiliman commented Nov 10, 2022

I submitted a PR to fix the invalid index handling. remix-run/remix#4560

@kentcdodds
Copy link

Thank you!

@kiliman
Copy link
Owner

kiliman commented Nov 10, 2022

While the PR is being reviewd, I'm going to update flat routes to add the /index suffix that Remix is currently looking for.

@kiliman
Copy link
Owner

kiliman commented Nov 10, 2022

@nakleiderer @lukebowerman @puchesjr @shadyendless

Please try latest v0.4.8. I added a workaround in the package itself until the PR bug fix has been merged into Remix.

Thanks everyone for your patience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants