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

Unexpected route id inconsistency #9591

Open
juanbercoff opened this issue Jun 8, 2024 · 10 comments
Open

Unexpected route id inconsistency #9591

juanbercoff opened this issue Jun 8, 2024 · 10 comments

Comments

@juanbercoff
Copy link

juanbercoff commented Jun 8, 2024

Reproduction

https://stackblitz.com/edit/remix-run-remix-5qqadr?file=vite.config.ts,app%2Froutes%2Fdashboard%2Froute.tsx,app%2Froutes%2Fdashboard%2Fchild%2Froute.tsx

System Info

import { vitePlugin as remix } from '@remix-run/dev';
import { defineRoutes } from '@remix-run/dev/dist/config/routes';
import { defineConfig } from 'vite';
import tsconfigPaths from 'vite-tsconfig-paths';

export default defineConfig({
  plugins: [
    remix({
      future: {
        v3_fetcherPersist: true,
        v3_relativeSplatPath: true,
        v3_throwAbortReason: true,
      },
      routes(defineRoutes) {
        return defineRoutes((route) => {
          route('dashboard', 'routes/dashboard/route.tsx', () => {
            route('child', 'routes/dashboard/child/route.tsx');
          });
        });
      },
    }),

    tsconfigPaths(),
  ],
});

Used Package Manager

npm

Expected Behavior

The id of the parent route should be the same regardless the route being rendered.

Actual Behavior

When rendering the dashboard path, the id of the route is routes/dashboard, but when rendering the child route, dashboard/child the id of the parent route is routes/dashboard/route.

Should the id always be the same relative to app?

@juanbercoff juanbercoff changed the title Route Id Unexpected route id inconsistency Jun 8, 2024
@kiliman
Copy link
Collaborator

kiliman commented Jun 9, 2024

The route ID is the filename relative to app folder without the file extension.

@akomm
Copy link

akomm commented Jun 10, 2024

update NVM, it makes sense:

Wait, it actually makes sense. Just confusing that the example mixes (custom) folder logic.

routes/_index.tsx as file, equivalent folder would be routes/_index/route.tsx to the ID would be routes/_index for both.

previous post:
@kiliman I've created a reproduction that is a bit easier to use:

Remix DOC on ID: https://remix.run/docs/en/main/hooks/use-route-loader-data
Blitzstack: https://stackblitz.com/edit/remix-run-remix-mmhd6w

Maybe remix did there some BC break within non-major version, but as I've mentioned in react-flat-routes before and you verified, the ID is the path of the file, not the pathname (typically including the filename itself). The example in the docs just provides file a file route example, not using folders.

But it makes sense, that the same route, expressed differently, should have the same ID, right?

So app/routes/teams.tsx and app/routes/teams/route.tsx should have the same id: routes/teams, not routes/teams and routes/teams/route.

That means, which I agree feels a bit weird, a top level _index route should have an ID of "routes".

@sergiodxa
Copy link
Member

I think the issue is you're using code to define the routes, here Remix infer the id from the file without the extension, the special route file inside a folder is not something that applies anymore since you could name your route file with any name because you're using code.

So yeah this routes/dashboard/child/route.tsx will become routes/dashboard/child/route because it just removed the extension, if you wanted to get routes/dashboard/child you can pass an id yourself by doing route('child', 'routes/dashboard/child/route.tsx', { id: "routes/dashboard/child" }).

For the default convention I expect the /route to not be included and I think it's what happens, but when using code that doesn't apply anymore.

@akomm
Copy link

akomm commented Jun 10, 2024

When switching back to default routing everything is consistent, which confirms sergiodxa's statement:
https://stackblitz.com/edit/remix-run-remix-1pd9fm

So when you make custom route handling, you need to make sure to keep the ID consistency and ensure that they are unique at the end - makes sense.

@juanbercoff
Copy link
Author

This is the config with the explicit ids.
image

When you navigate to dashboard/child, the id is different that when navigating to dashboard.
image

@akomm
Copy link

akomm commented Jun 10, 2024

This is because of this result (note the dashboard is present twice):
grafik

I think the reason for this is, that in defineRoutes you don't overwrite everything starting from "root" and your dashboard/route.tsx is compatible with the default remix routing. So what happens is remix picks up the remix compatible dashboard/route.tsx and then adds your routing definitions on top.

The dashboard is not duplicated if you specify the id that remix would naturally generate. The only way I've found to opt-out of remix's default routing to still run is create a custom routes folder:

https://stackblitz.com/edit/remix-run-remix-mg5olc?file=vite.config.ts

@juanbercoff
Copy link
Author

Yep, it looks like it. I think there should be a note on the docs to replace the whole tree to avoid this kind of issues,

@brophdawg11
Copy link
Contributor

We should warn about this - the solution arrived at above is accurate - if you want to manually define child routes of an existing file-based parent route, you will need to specify the parent route with the same ID that Remix uses on the file-system.

But our code should be smart enough to warn/error if a dup route is found so you don't end up with 2 path="dashboard" routes in the tree.

Leaving this open so we can take a look. The check would likely make sense to do here if anyone wants to take a stab. We should be able to detect if we're about to add a config-based route that has the same parentId/path/index values as an existing route.

@akomm
Copy link

akomm commented Jun 13, 2024

Do you think it might make sense to have an option to opt-out of remix default routing, so if someone wants to implement a custom solution there is no chance of accidentally leak remix logic into own implementation? Especially when there are changes in the future major releases. The error message is nice, but only triggers when the case is tested. When a future change adds new potential cases, someone would have to think it all through to figure out what can happen. With this is disabled for a custom routing, only evtl. API changes (hopefully typed) need to be addressed.

@sergiodxa
Copy link
Member

You can use ignoredRouteFiles: ['**/*'] to ignore every file inside app/routes and then use the routes function to define your routes by hand.

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

No branches or pull requests

5 participants