epicweb-dev / epic-stack

This is a Full Stack app starter with the foundational things setup and configured for you to hit the ground running on your next EPIC idea.
https://www.epicweb.dev/epic-stack
MIT License
4.32k stars 355 forks source link

circular dependencies #770

Closed fredericrous closed 2 months ago

fredericrous commented 2 months ago

I don't know if that's so much an issue but I noticed circular dependencies while trying to apply the import/no-cycle eslint rule


app/utils/connections.server.ts:

import { GitHubProvider } from './providers/github.server.ts'

app/utils/providers/github.server.ts

import { connectionSessionStorage } from '../connections.server.ts'

app/routes/_auth+/verify.server.ts

import { handleVerification as handleChangeEmailVerification } from '#app/routes/settings+/profile.change-email.server.tsx'

app/routes/settings+/profile.change-email.tsx

import {
  prepareVerification,
  requireRecentVerification,
} from '#app/routes/_auth+/verify.server.ts'

app/routes/_auth+/verify.server.ts

import {
  VerifySchema,
  codeQueryParam,
  redirectToQueryParam,
  targetQueryParam,
  typeQueryParam,
  type VerificationTypes,
} from './verify.tsx'

app/routes/_auth+/verify.tsx

import { validateRequest } from './verify.server.ts'

app/routes/_auth+/verify.server.ts

import {
  handleVerification as handleLoginTwoFactorVerification,
  shouldRequestTwoFA,
} from './login.server.ts'

app/routes/_auth+/login.server.ts

import { getRedirectToUrl, type VerifyFunctionArgs } from './verify.server.ts'

the full log

app/routes/_auth+/login.server.ts
  12:1  error  Dependency cycle detected  import/no-cycle

app/routes/_auth+/verify.server.ts
   5:1  error  Dependency cycle detected  import/no-cycle
  14:1  error  Dependency cycle detected  import/no-cycle
  20:1  error  Dependency cycle detected  import/no-cycle

app/routes/_auth+/verify.tsx
  14:1  error  Dependency cycle detected  import/no-cycle

app/routes/settings+/profile.change-email.server.tsx
   4:1  error  Dependency cycle detected  import/no-cycle
  12:1  error  Dependency cycle detected  import/no-cycle

app/routes/settings+/profile.change-email.tsx
  17:1  error  Dependency cycle via #app/routes/settings+/profile.change-email.server.tsx:5  import/no-cycle

apps/frontjutsu.com/app/utils/connections.server.ts
  3:1  error  Dependency cycle detected  import/no-cycle

apps/frontjutsu.com/app/utils/providers/github.server.ts
  6:1  error  Dependency cycle detected  import/no-cycle

✖ 10 problems (10 errors, 0 warnings)

line numbers could be a bit off

kentcdodds commented 2 months ago

What do you recommend?

fredericrous commented 2 months ago

depends on the situation. If you want to add more features to the template in a near future. Or if others using this template are reading this thread, I recommend to:

  1. enable https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/no-cycle.md
  2. ignore the circular dependency error with a eslint-disable-next-line comment

This way there should be no additional circular dependency added. That's what I'm gonna do.

I'm still on eslint 8, I do not know yet how to configure this rule on eslint 9. I haven't switched yet, I'm waiting for more plugins to be available.

When my mvp will finally go to production, I will address these issues and will be happy to open a PR. But I still have a long way to go. It's low priority on my backlog

In the mid time if someone picks up the task, the solution to a circular dependency between 2 files is usually to create a 3rd one, with in it the function that these files both use

kentcdodds commented 2 months ago

ESM has intentional support for these types of circular dependencies so I think it's probably fine to just ignore.