kiliman / remix-flat-routes

Remix package to define routes using the flat-routes convention
MIT License
695 stars 24 forks source link

Form submitting to incorrect routes #14

Closed nakleiderer closed 1 year ago

nakleiderer commented 2 years ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

https://user-images.githubusercontent.com/1154812/199886355-97e0266f-4b2f-47d9-bd53-28e3f735fe48.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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

lukebowerman commented 1 year ago

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: https://github.com/remix-run/remix/pull/4376 (deals with index matching so perhaps flat-routes is interacting with this unexpectedly?)

kiliman commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

I submitted a PR to fix the invalid index handling. https://github.com/remix-run/remix/pull/4560

kentcdodds commented 1 year ago

Thank you!

kiliman commented 1 year ago

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 commented 1 year ago

@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.