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

Cyclic dependency while importing a route object to a corresponding component module #2755

Open
artptr opened this issue Nov 14, 2024 · 12 comments
Labels
question This issue is about a user needing insight

Comments

@artptr
Copy link

artptr commented Nov 14, 2024

Which project does this relate to?

Router

Describe the bug

Calling hooks from a route object causes a cyclic dependency issue. This problem arises because importing hooks into the route object requires importing the component itself. This import chain creates a circular dependency, where the component imports the route module to access the route object.

// routes.ts

// ESLint: Dependency cycle via ./Foo.tsx (import/no-cycle)
import { Foo } from './Foo.tsx';

export const fooRoute = createRoute({
    ...
    component: Foo,
});
// Foo.tsx

// ESLint: Dependency cycle via ./routes (import/no-cycle)
import { fooRoute } from './routes.ts';

export const Foo = () => {
    const ctx = fooRoute.useRouteContext();
    ...
};

I assume the importance of type safety, but circular dependencies can also lead to test failures in Jest when attempting to mock components or functions involved in a cycle.

Your Example Website or App

Steps to Reproduce the Bug or Issue

  1. Define a route and associate it with a component that uses hooks from the route object.
  2. Import the hook(s) into the route definition to use within the component.
  3. Import the component back into the module where the route is created, so the component can access its route definition.

Expected behavior

The component should be able to access route-dependent hooks without creating cyclic dependencies.

Screenshots or Videos

No response

Platform

  • "@tanstack/react-router": "1.81.6"
  • "eslint": "9.14.0"
  • "eslint-plugin-import": "2.31.0"

Additional context

No response

@artptr artptr changed the title Cyclic Dependency Issue with Hook Imports in Route Objects Cyclic dependency while importing a route object to a corresponding component module Nov 14, 2024
@schiller-manuel
Copy link
Contributor

if you have the component defined in a another file, use the "global" hooks (e.g. useRouteContext()) with from set or use getRouteApi.

@artptr
Copy link
Author

artptr commented Nov 14, 2024

@schiller-manuel Oh, I see, thank you. Unfortunately, I have another error while using the global useRouteContext or getRouteApi:

// constants.ts

export const FOO_ROUTE_ID = 'foo';
// routes.ts

import { FOO_ROUTE_ID } from './constants.ts';
import { Foo } from './Foo.tsx';

export const fooRoute = createRoute({
    id: FOO_ROUTE_ID,
    component: Foo,
});
// Foo.tsx

import { FOO_ROUTE_ID } from './constants.ts';

export const Foo = () => {
    // TS2345: Argument of type 'foo' is not assignable to parameter of type Constrain<'foo', '__root__' | '/foo'>
    const { useRouteContext } = getRouteApi(FOO_ROUTE_ID);

    // or

    // TS2820: Type 'foo' is not assignable to type '__root__' | '/foo'. Did you mean '/foo'?
    const ctx = useRouteContext({ from: FOO_ROUTE_ID });
    ...
};

@schiller-manuel
Copy link
Contributor

looks like you would need to use /foo instead of foo? could be a bug. please provide a complete minimal example, ideally by forking one of the existing examples on stackblitz

@artptr
Copy link
Author

artptr commented Nov 14, 2024

https://stackblitz.com/edit/tanstack-router-hk6zmr?file=src%2Fmain.tsx

It seems there is the same concatenating for constants as used for paths. For example, if aboutRoute is a child of indexRoute, TypeScript will expect /index-route-id/about-route-id for from.

@SeanCassiere
Copy link
Member

@artptr the expected route ids are mentioned in the inferred types.

Image

@SeanCassiere SeanCassiere added the question This issue is about a user needing insight label Nov 15, 2024
@artptr
Copy link
Author

artptr commented Nov 15, 2024

@SeanCassiere Yes, but I would expect that it will be the same id the route was created with. Am I wrong about it?

@SeanCassiere
Copy link
Member

SeanCassiere commented Nov 15, 2024

Yes, the inferred types will always reflect the actual route IDs that'd be present during runtime.

Edit:
Sidenote, you shouldn't be providing both an id and a path. You'll probably get an invariant error during runtime when both are supplied. IDs are only meant to be used on pathless routes.

@artptr
Copy link
Author

artptr commented Nov 15, 2024

Documentation says that id property is "optional, but required if a path is not provided". I can conclude it's optional if path is provided. Anyway, it should be possible to set an arbitrary id to any route to be able to use the route API as it seems to me.

@SeanCassiere
Copy link
Member

With regards to the documentation, we'd just need to update the verbiage in the documentation to make this distinctio of the runtime behaviour clearer.


This restriction of not allowing both an id and a path has been present in Router for quite a while. This is the block of code performing the invariant call.

invariant(
!((options as any)?.id && (options as any)?.path),
`Route cannot have both an 'id' and a 'path' option.`,
)

@schiller-manuel Your recent changes in the generator (#2597) made it so that the routeTree file used a .update() call to set the id on the route, which bypasses the invariant call in the constructor. Would removing the restriction in the constructor here be "safe"?

@schiller-manuel
Copy link
Contributor

hm maybe we should just add the same invariant into the update() method?

@SeanCassiere
Copy link
Member

hm maybe we should just add the same invariant into the update() method?

Adding that logic into the update method would certainly break the work done in #2597.

@artptr
Copy link
Author

artptr commented Nov 17, 2024

Is there any technical restriction to use id for pathful routes too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question This issue is about a user needing insight
Projects
None yet
Development

No branches or pull requests

3 participants