-
Notifications
You must be signed in to change notification settings - Fork 751
build(lint): add rule to prefer direct imports over index.ts #6372
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
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
| 'Avoid child_process, use ChildProcess from `shared/utilities/processUtils.ts` instead.', | ||
| }, | ||
| { | ||
| name: '..', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This prevents importing from index.ts files in other paths? E.g. ./adjacentFolder/folderWithIndex/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, but I think the circular dependency is only a problem when the index.ts is importing from the current file as well. This should only happen when the index.ts is a parent rather than an adjacent folder.
There is another case where the index.ts is more than one level up, but I wasn't able to find any instances of this in the toolkit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've found the most common pitfall to be having a file that depends on an index.ts that is exporting something depends on your file. E.g.
// connection.ts
import { logger } from './logger' // index.ts import
// logger/index.ts
export logger from './logger'
export loggerUtil from './loggerUtil'
// logger/loggerUtil.ts
import { connectionType } from '../connection'
connection.ts > logger/index.ts > loggerUtils.ts > connection.ts,
when in reality we just want connection.ts > logger/logger.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Yeah unfortunately, this won't address that. I would also imagine it would be hard to catch via lint rule.
justinmk3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice solution!
## Problem Importing from `..` or an `index.ts` file can lead to circular dependencies. ## Solution - add a lint rule to discourage importing from `..`. - migrate existing cases to import directly from the target module.
## Problem Importing from `..` or an `index.ts` file can lead to circular dependencies. ## Solution - add a lint rule to discourage importing from `..`. - migrate existing cases to import directly from the target module.
## Problem Importing from `..` or an `index.ts` file can lead to circular dependencies. ## Solution - add a lint rule to discourage importing from `..`. - migrate existing cases to import directly from the target module.
Problem
Importing from
..or anindex.tsfile can lead to circular dependencies.Solution
...feature/xbranches will not be squash-merged at release time.