-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat: no-token-import rule #103889
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: no-token-import rule #103889
Changes from all commits
9dd0f8a
e9e94bf
10a197c
70d7751
d752096
eaeb67f
ae56e11
591b66a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -303,6 +303,7 @@ const config: Config.InitialOptions = { | |
| transform: { | ||
| '^.+\\.jsx?$': ['babel-jest', babelConfig as any], | ||
| '^.+\\.tsx?$': ['babel-jest', babelConfig as any], | ||
| '^.+\\.mjs?$': ['babel-jest', babelConfig as any], | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. jest can’t read mjs so we now transform it with babel like all other ts files |
||
| '^.+\\.pegjs?$': '<rootDir>/tests/js/jest-pegjs-transform.js', | ||
| }, | ||
| transformIgnorePatterns: [ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| import noTokenImport from './no-token-import.mjs'; | ||
|
|
||
| export const rules = { | ||
| 'no-token-import': noTokenImport, | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| const TOKEN_PATH = 'utils/theme/scraps'; | ||
| const EXCEPT_DIR_NAME = 'static/app/utils/theme'; | ||
|
|
||
| /** | ||
| * | ||
| * @param {unknown} importPath | ||
| * @returns {boolean} | ||
| */ | ||
| function isForbiddenImportPath(importPath) { | ||
| if (typeof importPath !== 'string') return false; | ||
|
|
||
| return importPath.includes(TOKEN_PATH); | ||
| } | ||
|
|
||
| /** | ||
| * @type {import('eslint').Rule.RuleModule} | ||
| */ | ||
| const noTokenImport = { | ||
| meta: { | ||
| type: 'problem', | ||
| docs: { | ||
| description: `Disallow imports from "${TOKEN_PATH}" except within a directory named "${EXCEPT_DIR_NAME}".`, | ||
| recommended: false, | ||
| }, | ||
| schema: [], | ||
| messages: { | ||
| forbidden: `Do not import scraps tokens directly - prefer using theme tokens.`, | ||
| }, | ||
| }, | ||
|
|
||
| create(context) { | ||
| const importerIsInAllowedDir = context.filename?.includes(EXCEPT_DIR_NAME); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Path matching allows unintended directoriesThe |
||
|
|
||
| return { | ||
| ImportDeclaration(node) { | ||
| if (!node || node.source.type !== 'Literal') return; | ||
| if (importerIsInAllowedDir) return; | ||
|
|
||
| const value = node.source.value; | ||
|
|
||
| if (isForbiddenImportPath(value)) { | ||
| context.report({ | ||
| node, | ||
| messageId: 'forbidden', | ||
| }); | ||
| } | ||
| }, | ||
| }; | ||
| }, | ||
| }; | ||
|
|
||
| export default noTokenImport; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| import {RuleTester} from 'eslint'; | ||
|
|
||
| import noTokenImport from './no-token-import.mjs'; | ||
|
|
||
| const ruleTester = new RuleTester(); | ||
|
|
||
| ruleTester.run('no-token-import', noTokenImport, { | ||
| valid: [ | ||
| { | ||
| code: 'import x from "other-package";', | ||
| filename: '/project/src/foo/file.ts', | ||
| }, | ||
| { | ||
| code: 'const x = require("other-package");', | ||
| filename: '/project/src/foo/file.js', | ||
| }, | ||
|
|
||
| { | ||
| code: 'import {colors} from "sentry/utils/theme/scraps/colors";', | ||
| filename: '/static/app/utils/theme/theme.tsx', | ||
| }, | ||
| ], | ||
|
|
||
| invalid: [ | ||
| { | ||
| code: 'import {colors} from "sentry/utils/theme/scraps/colors";', | ||
| filename: '/static/app/index.tsx', | ||
| errors: [{messageId: 'forbidden'}], | ||
| }, | ||
| ], | ||
| }); |
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. jest is just so weird.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you could change the test env for this test, doesn't need to be jsdom https://jestjs.io/docs/configuration#testenvironment-string but some of the beforeEach etc might all break i guess
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I tried that and was hit with |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |
| "static/app", | ||
| "static/gsApp", | ||
| "static/gsAdmin", | ||
| "static/eslint", | ||
| "tests/js", | ||
| "config", | ||
| "scripts" | ||
|
|
||
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.
we can’t to
.tsbecause that would require a build-step for the eslint config 😭