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

feat(eslint-plugin): [no-circular-imports] add new rule #8965

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

yeonjuan
Copy link
Contributor

@yeonjuan yeonjuan commented Apr 22, 2024

PR Checklist

Overview

In this pr, I implemented no-circular-import which checks circular-import using the information that TS has. see #224 (comment).

The current implementation lacks some handling compared to eslint-plugin-import/no-cycle. (dynamic-import, require..).

Before go any further, I have a question about the case with dynamic-import.

With this implementation way, it would be difficult to checks for modules that reference each other via dynamic import. Actually there is a way to traverse the ASTs in the TS to check for dynamic imports, but performance may be poor.

// foo.ts
const foo = () => import("./bar.ts");

// bar.ts
const bar = () => import("./foo.ts");

Should this be left as an edge case?


I've already left an inquiry on discord about this. I'd be interested to hear what other members think.

I'm not sure if dynamic imports should be warned at all because (a) there are many ways they are not statically analyzable (b) one of their use cases is to avoid circular imports (by deferring them) - Josh-Cena

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @yeonjuan!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

Copy link

netlify bot commented Apr 22, 2024

Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit 82f36a9
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/662be03a2275f900088944af
😎 Deploy Preview https://deploy-preview-8965--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 97 (🟢 up 4 from production)
Accessibility: 100 (no change from production)
Best Practices: 92 (no change from production)
SEO: 98 (no change from production)
PWA: 80 (no change from production)
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@yeonjuan yeonjuan force-pushed the circular-import branch 2 times, most recently from 58bcfba to 780fb1a Compare April 22, 2024 16:24
@yeonjuan yeonjuan marked this pull request as ready for review April 26, 2024 17:11
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

Successfully merging this pull request may close these issues.

Rule proposal: detect and warn about circular imports
1 participant