Skip to content

Commit e17ec1d

Browse files
hayemaxihayemaxi
committed
ci(lint): rule to detect index.ts imports in core
I could not find a lint rule (or a configuration of a lint rule) to achieve this, so I just made one. I tried some things from eslint-plugin-import (namely no-cycles), but I was not happy with the results. This will prevent you from importing from index.ts files while working in packages/core/, which has been a source of annoying and hard to understand circular dependency issues. It only affects imports in core, and only in src/ dirctories that do not start with test/
1 parent 9dffe3e commit e17ec1d

File tree

7 files changed

+209
-0
lines changed

7 files changed

+209
-0
lines changed

‎.eslintrc.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -180,6 +180,7 @@ module.exports = {
180180
'aws-toolkits/no-console-log': 'error',
181181
'aws-toolkits/no-json-stringify-in-log': 'error',
182182
'aws-toolkits/no-printf-mismatch': 'error',
183+
'aws-toolkits/no-index-import': 'error',
183184
'no-restricted-imports': [
184185
'error',
185186
{

‎plugins/eslint-plugin-aws-toolkits/index.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import NoStringExecForChildProcess from './lib/rules/no-string-exec-for-child-pr
1111
import NoConsoleLog from './lib/rules/no-console-log'
1212
import noJsonStringifyInLog from './lib/rules/no-json-stringify-in-log'
1313
import noPrintfMismatch from './lib/rules/no-printf-mismatch'
14+
import noIndexImport from './lib/rules/no-index-import'
1415

1516
const rules = {
1617
'no-await-on-vscode-msg': NoAwaitOnVscodeMsg,
@@ -21,6 +22,7 @@ const rules = {
2122
'no-console-log': NoConsoleLog,
2223
'no-json-stringify-in-log': noJsonStringifyInLog,
2324
'no-printf-mismatch': noPrintfMismatch,
25+
'no-index-import': noIndexImport,
2426
}
2527

2628
export { rules }

‎plugins/eslint-plugin-aws-toolkits/lib/rules/no-index-import.ts‎

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
/*!
2+
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
import { ESLintUtils, TSESTree } from '@typescript-eslint/utils'
7+
import { Rule } from 'eslint'
8+
import path from 'path'
9+
// eslint-disable-next-line no-restricted-imports
10+
import * as fs from 'fs'
11+
12+
export const errMsg =
13+
'do not import from folders or index.ts files since it can cause circular dependencies. import from the file directly.'
14+
15+
/**
16+
* Prevents TS imports from index.ts in packages/core/src (test files ignored).
17+
* It is easy to create cryptic circular dependency issues in packages/core due to the current import/export structure.
18+
*
19+
* Example:
20+
* - index.ts
21+
* export Auth from './auth'
22+
* export Connection from './connection'
23+
* - auth.ts
24+
* export class Auth
25+
* - connection.ts
26+
* import { Auth } from '.' // circular dependency
27+
* export type Connection
28+
*
29+
* This is a problem because connection.ts depends on index.ts to export 'Auth', which depends on connection.ts
30+
* to export `Connection`.
31+
*/
32+
export default ESLintUtils.RuleCreator.withoutDocs({
33+
meta: {
34+
docs: {
35+
description: 'disallow importing from index.ts files in packages/core/src/',
36+
recommended: 'recommended',
37+
},
38+
messages: {
39+
default: errMsg,
40+
},
41+
type: 'problem',
42+
fixable: 'code',
43+
schema: [],
44+
},
45+
defaultOptions: [],
46+
create(context) {
47+
const filePath = context.physicalFilename
48+
if (!filePath.match(/packages\/core\/src\/(?!test)/)) {
49+
// only trigger for import statements inside packages/core/ src files,
50+
// but don't include test files
51+
return {}
52+
}
53+
54+
return {
55+
ImportDeclaration(node: TSESTree.ImportDeclaration) {
56+
const relativeImportPath = node.source.value
57+
58+
// likely importing from some external module or is an irrelevant file
59+
if (
60+
!relativeImportPath.startsWith('.') ||
61+
['.json', '.js', '.vue'].includes(path.extname(relativeImportPath))
62+
) {
63+
return
64+
}
65+
66+
const absoluteImportPath = path
67+
.resolve(path.dirname(filePath), relativeImportPath)
68+
.replace(/(\.d)?\.ts/, '') // remove any .d.ts or .ts file extensions on the import (unlikely)
69+
70+
if (path.basename(absoluteImportPath) !== 'index') {
71+
// Not an index.ts file but is some typescript file.
72+
if (['.ts', '.d.ts'].some((e) => fs.existsSync(absoluteImportPath + e))) {
73+
return
74+
}
75+
76+
// If if does not exist as a folder, then the path is simply wrong. Another, more descriptive error will surface instead.
77+
if (!fs.existsSync(absoluteImportPath)) {
78+
return
79+
}
80+
}
81+
82+
return context.report({
83+
node: node.source,
84+
messageId: 'default',
85+
// TODO: We can add a fixer to resolve the import for us.
86+
// fix: (fixer) => {
87+
// // somehow parse the exports of the imported index.ts file to get the actual export path,
88+
// // then replace the index import with it.
89+
// return fixer.replaceText(node.source, newImport)
90+
// },
91+
})
92+
},
93+
}
94+
},
95+
}) as unknown as Rule.RuleModule

‎plugins/eslint-plugin-aws-toolkits/test/resources/noIndexImports/index.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
/*!
2+
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
export { doNothing } from './utils'

‎plugins/eslint-plugin-aws-toolkits/test/resources/noIndexImports/types.d.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
/*!
2+
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
export interface doNothing {}

‎plugins/eslint-plugin-aws-toolkits/test/resources/noIndexImports/utils.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
/*!
2+
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
export function doNothing() {}

‎plugins/eslint-plugin-aws-toolkits/test/rules/no-index-import.test.ts‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
/*!
2+
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
import path from 'path'
7+
import { rules } from '../../index'
8+
import { errMsg } from '../../lib/rules/no-index-import'
9+
import { getRuleTester } from '../testUtil'
10+
11+
// Linted files need `packages/core/src` in their filepaths for them to pass, but we are referencing test resource files
12+
// from this project directory. So, "inject" the expected substring into this filepath.
13+
const verboseFilePath = `${__dirname.replace('dist/', '')}/../../../../packages/core/src/../../../plugins/eslint-plugin-aws-toolkits/test/rules/${path.basename(__filename.replace('.js', '.ts'))}`
14+
15+
getRuleTester().run('no-index-import', rules['no-index-import'], {
16+
valid: [
17+
{
18+
code: "import { doNothing } from '../resources/noIndexImports/utils'",
19+
filename: verboseFilePath,
20+
},
21+
{
22+
code: "import { doNothing } from '../resources/noIndexImports/utils.ts'",
23+
filename: verboseFilePath,
24+
},
25+
{
26+
code: "import config from '.eslintrc'",
27+
filename: 'packages/amazonq/src/test.ts',
28+
},
29+
{
30+
code: "import ts from '@typescript'",
31+
filename: verboseFilePath,
32+
},
33+
{
34+
code: "import { doNothing } from '../resources/noIndexImports'",
35+
filename: verboseFilePath.replace('packages/core/src/', 'packages/core/src/test/../'),
36+
},
37+
{
38+
code: "import { doNothing } from '../resources/noIndexImports'",
39+
filename: verboseFilePath.replace('packages/core/src/', 'packages/core/src/testInteg/../'),
40+
},
41+
{
42+
code: "import { doNothing } from '../resources/noIndexImports/utils'",
43+
filename: verboseFilePath.replace('packages/core/src/', 'packages/core/src/index/../'),
44+
},
45+
{
46+
code: "import { doNothing } from '../resources/noIndexImports/types.d.ts'",
47+
filename: verboseFilePath,
48+
},
49+
{
50+
code: "import { doNothing } from '../resources/noIndexImports/types'",
51+
filename: verboseFilePath,
52+
},
53+
{
54+
code: "import { doNothing } from '../resources/noIndexImports/doesNotExist.json'",
55+
filename: verboseFilePath,
56+
},
57+
{
58+
code: "import { doNothing } from '../resources/noIndexImports/utils.js'",
59+
filename: verboseFilePath,
60+
},
61+
{
62+
code: "import { doNothing } from '../resources/noIndexImports/utils.vue'",
63+
filename: verboseFilePath,
64+
},
65+
{
66+
code: "import { doNothing } from '../resources/nonexistantpath'",
67+
filename: verboseFilePath,
68+
},
69+
],
70+
71+
invalid: [
72+
{
73+
code: "import { doNothing } from '../resources/noIndexImports'",
74+
errors: [errMsg],
75+
filename: verboseFilePath,
76+
},
77+
{
78+
code: "import { doNothing } from '../resources/noIndexImports/'",
79+
errors: [errMsg],
80+
filename: verboseFilePath,
81+
},
82+
{
83+
code: "import { doNothing } from '../resources/noIndexImports/index'",
84+
errors: [errMsg],
85+
filename: verboseFilePath,
86+
},
87+
{
88+
code: "import { doNothing } from '../resources/noIndexImports/index.ts'",
89+
errors: [errMsg],
90+
filename: verboseFilePath,
91+
},
92+
],
93+
})

0 commit comments

Comments
 (0)