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.
| import { doT } from '../imports'; | ||
| import constants from '../constants'; |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Confirm doT is (named) re-exported from lib/core/imports
The original code consumed the default export from @deque/dot.
After the refactor we import a named symbol:
import { doT } from '../imports';If lib/core/imports/index.js (or the resolved file) does not re-export doT with the same structure ({ compile() { … } }) every runtime call to doT.compile(...) (e.g. lines 605, 637, 657, 671) will explode with “compile is not a function”.
Run the quick check below – if nothing is printed the symbol is missing or not exported as named:
If no lines are reported, we need either:
-import { doT } from '../imports';
+import doT from '../imports';or we must add export const doT = … into lib/core/imports.
🏁 Script executed:
#!/usr/bin/env bash
# Verify that a named export `doT` exists under lib/core/imports
fd --extension js 'lib/core/imports' -x \
node - <<'PY'
import importlib.util, sys, pathlib, inspect, json, os
target = pathlib.Path('lib/core/imports')
for p in target.rglob('*.js'):
mod_name = p.stem
# crude static detection – look for `export.*doT`
with open(p) as f:
src = f.read()
if 'export' in src and 'doT' in src:
print(f"Found possible doT export in {p}")
PYLength of output: 416
Let’s correct the search and look for any doT exports under lib/core/imports:
🏁 Script executed:
#!/usr/bin/env bash
# Search for named or default exports of doT in lib/core/imports
fd --extension js . lib/core/imports -x grep -Hn "export .*doT" {}
fd --extension js . lib/core/imports -x grep -Hn "export default .*doT" {}Length of output: 149
Ensure doT Is Actually Exported from lib/core/imports
I ran a check across lib/core/imports and found no evidence of either a named or default export for doT. As written, this line in lib/core/base/audit.js will fail at runtime:
import { doT } from '../imports';Please address one of the following:
• Add a named export in your imports entry point (lib/core/imports/index.js or equivalent):
// lib/core/imports/index.js
import doT from '@deque/dot';
export const doT = doT;• Or switch back to a default import and ensure you export default in the imports module:
// lib/core/base/audit.js
- import { doT } from '../imports';
+ import doT from '../imports';Either way, confirm that doT.compile(...) calls will resolve correctly.
🤖 Prompt for AI Agents (early access)
In lib/core/base/audit.js around lines 14 to 15, the import statement for doT uses a named import from '../imports', but doT is not currently exported as a named export in lib/core/imports. To fix this, update the exports in lib/core/imports/index.js (or the main imports file) to include a named export for doT by importing doT from '@deque/dot' and then exporting it as a named export. Alternatively, revert to a default import for doT and ensure the imports module exports doT as default. This will ensure that calls to doT.compile(...) in audit.js work correctly at runtime.
| { | ||
| // 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' | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
no-restricted-imports mis-configured – regex is not a supported key
patterns expects an array of strings or objects with a group field (see ESLint docs).
Objects of the form:
{
regex: '^[^.]',
message: '…'
}are silently ignored, so the new restriction will never fire.
A minimal fix:
- patterns: [
- {
- regex: '^[^.]',
- message: 'Only core/imports files should import from node modules'
- }
- ]
+ patterns: [
+ {
+ group: ['^[^.]'], // or simply '*' if you want all non-relative paths
+ message: 'Only core/imports files should import from node modules'
+ }
+ ]The same regex objects appear in multiple rule blocks (utils, public, reporters, commons, etc. ‑ see lines 163-276). Please adjust those as well, otherwise the lint step will pass but provide no protection.
🤖 Prompt for AI Agents (early access)
In eslint.config.js from lines 125 to 141, the no-restricted-imports rule incorrectly uses a regex key inside patterns, which ESLint does not support. Replace the regex key with a group key containing the regex string to properly enforce the restriction. Also, locate and update all similar occurrences of this misconfiguration in other rule blocks between lines 163 and 276 to ensure consistent linting behavior.
Adds eslint rules to restrict import paths from #4758, as well as virtual node use in utils.