-
Notifications
You must be signed in to change notification settings - Fork 840
chore: add eslint rules to enact import rules #4760
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
base: develop
Are you sure you want to change the base?
Changes from all commits
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 | ||||
---|---|---|---|---|---|---|
|
@@ -122,6 +122,182 @@ module.exports = [ | |||||
'no-use-before-define': 'off' | ||||||
} | ||||||
}, | ||||||
{ | ||||||
// disallow imports from node modules | ||||||
ignores: ['lib/core/imports/**/*.js'], | ||||||
rules: { | ||||||
'no-restricted-imports': [ | ||||||
'error', | ||||||
{ | ||||||
patterns: [ | ||||||
{ | ||||||
regex: '^[^.]', | ||||||
message: 'Only core/imports files should import from node modules' | ||||||
} | ||||||
] | ||||||
} | ||||||
] | ||||||
} | ||||||
}, | ||||||
dbjorge marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
{ | ||||||
// disallow imports in standards | ||||||
files: ['lib/standards/**/*.js'], | ||||||
// index file can import other standards and from utils | ||||||
ignores: ['lib/standards/index.js'], | ||||||
rules: { | ||||||
'no-restricted-imports': [ | ||||||
'error', | ||||||
{ | ||||||
patterns: [ | ||||||
{ | ||||||
group: ['*'], | ||||||
message: | ||||||
"Standard files shouldn't use imports as they are just hard coded data objects" | ||||||
} | ||||||
] | ||||||
} | ||||||
] | ||||||
} | ||||||
}, | ||||||
{ | ||||||
// restrict imports to core/utils files to other core/utils, core, core/base, standards, or reporters/helpers | ||||||
files: ['lib/core/utils/**/*.js'], | ||||||
rules: { | ||||||
'no-restricted-imports': [ | ||||||
'error', | ||||||
{ | ||||||
patterns: [ | ||||||
{ | ||||||
// e.g. "../commons/aria/" or "../public/" | ||||||
regex: | ||||||
'.*\\.\\.\\/(commons|imports|public|checks|rules)\\/|.*\\.\\.\\/reporters\\/.*?\\.js', | ||||||
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. This should probably also forbid importing from the index files of the forbidden directories (eg, to catch a normal usage of importing
Suggested change
(same comment for the other similar rules) |
||||||
message: | ||||||
'Util files should only import from other utils, core, or standard files' | ||||||
}, | ||||||
// disallow imports from node modules | ||||||
// seems only 1 regex pattern is allowed to match as not having this allows node module imports even while having the general rule above for all files) | ||||||
{ | ||||||
regex: '^[^.]', | ||||||
message: 'Only core/imports files should import from node modules' | ||||||
} | ||||||
] | ||||||
} | ||||||
] | ||||||
} | ||||||
}, | ||||||
{ | ||||||
// restrict imports to core/public files to other core/public, or imports allowed by core/utils | ||||||
files: ['lib/core/public/**/*.js'], | ||||||
rules: { | ||||||
'no-restricted-imports': [ | ||||||
'error', | ||||||
{ | ||||||
patterns: [ | ||||||
{ | ||||||
// e.g. "../commons/aria/" or "../checks/" | ||||||
regex: | ||||||
'.*\\.\\.\\/(commons|imports|checks|rules)\\/|.*\\.\\.\\/reporters\\/.*?\\.js', | ||||||
message: | ||||||
'Public files should only import from other public, util, core, or standard files' | ||||||
}, | ||||||
// disallow imports from node modules | ||||||
{ | ||||||
regex: '^[^.]', | ||||||
message: 'Only core/imports files should import from node modules' | ||||||
} | ||||||
] | ||||||
} | ||||||
] | ||||||
} | ||||||
}, | ||||||
{ | ||||||
// disallow imports in core/imports files to any non-node module | ||||||
files: ['lib/core/imports/**/*.js'], | ||||||
rules: { | ||||||
'no-restricted-imports': [ | ||||||
'error', | ||||||
{ | ||||||
patterns: [ | ||||||
{ | ||||||
// relative file paths | ||||||
regex: '\\\.\\\.\\/', | ||||||
message: 'Import files should only import from node modules' | ||||||
} | ||||||
] | ||||||
} | ||||||
] | ||||||
} | ||||||
}, | ||||||
{ | ||||||
// disallow imports in core/reporters files to any non-util file | ||||||
files: ['lib/core/reporters/**/*.js'], | ||||||
rules: { | ||||||
'no-restricted-imports': [ | ||||||
'error', | ||||||
{ | ||||||
patterns: [ | ||||||
{ | ||||||
// e.g. "../commons/aria/" or "../checks/" | ||||||
regex: '.*\\.\\.\\/(commons|base|imports|public|checks|rules)\\/', | ||||||
message: 'Reporter files should only import util functions' | ||||||
}, | ||||||
// disallow imports from node modules | ||||||
{ | ||||||
regex: '^[^.]', | ||||||
message: 'Only core/imports files should import from node modules' | ||||||
} | ||||||
] | ||||||
} | ||||||
] | ||||||
} | ||||||
}, | ||||||
{ | ||||||
// disallow imports in commons files to any check or rule | ||||||
files: ['lib/commons/**/*.js'], | ||||||
rules: { | ||||||
'no-restricted-imports': [ | ||||||
'error', | ||||||
{ | ||||||
patterns: [ | ||||||
{ | ||||||
// e.g. ../checks/" | ||||||
regex: '.*\\.\\.\\/(checks|rules)\\/', | ||||||
message: 'Commons files cannot import from checks and rules' | ||||||
}, | ||||||
// disallow imports from node modules | ||||||
{ | ||||||
regex: '^[^.]', | ||||||
message: 'Only core/imports files should import from node modules' | ||||||
} | ||||||
] | ||||||
} | ||||||
] | ||||||
} | ||||||
}, | ||||||
{ | ||||||
// generally don't use virtual node in utils | ||||||
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. What does "generally" mean? How will a future maintainer know whether to apply the rule in a specific case? This should either justify why not, or if there is no articulable justification and there are several exceptions already, it probably shouldn't be a rule. |
||||||
files: ['lib/core/utils/**/*.js'], | ||||||
// these are files with known uses of virtual node that are legacy before this rule was enforced | ||||||
ignores: [ | ||||||
'lib/core/utils/closest.js', | ||||||
'lib/core/utils/contains.js', | ||||||
'lib/core/utils/query-selector-all-filter.js', | ||||||
'lib/core/utils/selector-cache.js' | ||||||
], | ||||||
rules: { | ||||||
'no-restricted-syntax': [ | ||||||
'error', | ||||||
{ | ||||||
selector: 'MemberExpression[object.name=vNode]', | ||||||
message: "Don't use Virtual Node objects in utils." | ||||||
}, | ||||||
{ | ||||||
selector: 'MemberExpression[object.name=virtualNode]', | ||||||
message: "Don't use Virtual Node objects in utils." | ||||||
} | ||||||
] | ||||||
} | ||||||
}, | ||||||
{ | ||||||
files: ['doc/examples/chrome-debugging-protocol/axe-cdp.js'], | ||||||
languageOptions: { | ||||||
|
@@ -189,7 +365,7 @@ module.exports = [ | |||||
'build/tasks/aria-supported.js', | ||||||
'doc/api/*', | ||||||
'doc/examples/jest_react/*.js', | ||||||
'lib/core/imports/*.js', | ||||||
'lib/core/imports/polyfills.js', | ||||||
'lib/core/utils/uuid.js', | ||||||
'axe.js', | ||||||
'axe.min.js' | ||||||
|
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.
It would be nice if this could summarize why we use this pattern (either directly or with a reference to an appropriate readme section/issue); future maintainers are more likely to respect the rule if it's cleary why it exists.
(I think this is more important for this rule and for the "no vNode in utils" thing (see comment below) than the other cases; the other ones are more obviously enforcing a non-circular import structure, but those two rules aren't so obvious about why they're here)