chore: add eslint rules to enact import rules #4760
Conversation
WalkthroughThis update introduces stricter ESLint rules to enforce architectural boundaries regarding import patterns across the codebase. Several files have been refactored to source their dependencies from internal modules instead of external packages, aligning with the new linting constraints. The ESLint ignore list is also refined to target a specific file rather than a broader pattern. No changes are made to exported or public entities; all modifications are limited to configuration and import statements. Changes
Sequence Diagram(s)sequenceDiagram
participant File as Source File (e.g., utils, commons, base)
participant Imports as Internal Imports Module
participant External as External Package
File->>Imports: import { dependency } from '../imports'
Imports->>External: (internally sources dependency)
File-->>File: Uses dependency as before
This diagram illustrates the new import flow, where source files now import dependencies from a centralized internal module, which in turn manages access to external packages. This enforces stricter boundaries and centralized control over third-party dependencies. ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
eslint.config.js (5)
162-187: Same pattern object issue as aboveThe
regexkey is repeated here; please update togroup(or convert to plain pattern strings) to ensure the rule actually runs.
188-212: Repeated mis-configuration (regexkey) – fix required
213-230: Repeated mis-configuration (regexkey) – fix required
231-253: Repeated mis-configuration (regexkey) – fix required
255-276: Repeated mis-configuration (regexkey) – fix required
🧹 Nitpick comments (1)
eslint.config.js (1)
368-369: Duplicate ignore entry forlib/core/imports/polyfills.js
polyfills.jsis already whitelisted in the dedicated polyfill block (lines 317-321).
Keeping the duplicate in the global ignore list is harmless but clutters the config.- 'lib/core/imports/polyfills.js',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
eslint.config.js(2 hunks)lib/commons/text/has-unicode.js(1 hunks)lib/commons/text/remove-unicode.js(1 hunks)lib/core/base/audit.js(1 hunks)lib/core/utils/css-parser.js(1 hunks)lib/core/utils/memoize.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Security Review
🔇 Additional comments (5)
lib/commons/text/remove-unicode.js (1)
7-7: Good import refactoring to follow architectural boundariesThe import for
emojiRegexTexthas been updated to use the centralized imports module instead of directly importing from an external package. This aligns with the new ESLint rules and improves architectural consistency.lib/commons/text/has-unicode.js (1)
7-7: Consistent import path updateThe import for
emojiRegexTexthas been correctly updated to use the internal imports module, matching the same pattern applied in other files. This ensures consistent dependency management and complies with the new ESLint rules.lib/core/utils/css-parser.js (1)
1-1: Import refactored to use centralized imports moduleThe import for
CssSelectorParserhas been updated to use the internal imports module instead of directly importing from an external package. This change aligns with the PR objectives and follows the same pattern as other import updates.lib/core/utils/memoize.js (1)
1-1: Import path updated to follow architectural patternThe import for
memoizehas been properly updated to use the internal imports module, which centralizes external dependencies. This helps enforce the architectural boundaries specified in the new ESLint rules.eslint.config.js (1)
277-300: Virtual-node ban looks good – legacy exceptions documentedThe explicit allow-list for the four legacy files plus the generic ban elsewhere is clear and will prevent future regressions.
| patterns: [ | ||
| { | ||
| regex: '^[^.]', | ||
| message: 'Only core/imports files should import from node modules' |
There was a problem hiding this comment.
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)
| { | ||
| // e.g. "../commons/aria/" or "../public/" | ||
| regex: | ||
| '.*\\.\\.\\/(commons|imports|public|checks|rules)\\/|.*\\.\\.\\/reporters\\/.*?\\.js', |
There was a problem hiding this comment.
This should probably also forbid importing from the index files of the forbidden directories (eg, to catch a normal usage of importing from '../imports')
| '.*\\.\\.\\/(commons|imports|public|checks|rules)\\/|.*\\.\\.\\/reporters\\/.*?\\.js', | |
| '.*\\.\\.\\/(commons|imports|public|checks|rules)(\\/|$)|.*\\.\\.\\/reporters\\/.*?\\.js', |
(same comment for the other similar rules)
| } | ||
| }, | ||
| { | ||
| // generally don't use virtual node in utils |
There was a problem hiding this comment.
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.
Adds eslint rules to restrict import paths from #4758, as well as virtual node use in utils.