feat: port eslint-plugin-barrel-files #304
Conversation
|
|
Warning Rate limit exceeded@43081j has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 3 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis update introduces four new ESLint rules focused on improving module import/export practices, particularly around barrel files and namespace imports. The rules are: forbidding the creation of barrel files, restricting imports from barrel files that cause large module graphs, disallowing namespace imports, and preventing re-exporting all exports from another module. Each rule is accompanied by detailed documentation and comprehensive test suites. The package dependencies are updated to include a utility for barrel file analysis. The MIT license is also amended to add a new copyright. Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant ESLint
participant RuleModule
Developer->>ESLint: Runs lint on codebase
ESLint->>RuleModule: Applies avoid-barrel-files
RuleModule-->>ESLint: Reports if module is a barrel file
ESLint->>RuleModule: Applies avoid-importing-barrel-files
RuleModule-->>ESLint: Reports if import triggers large module graph
ESLint->>RuleModule: Applies avoid-namespace-import
RuleModule-->>ESLint: Reports if namespace import is used
ESLint->>RuleModule: Applies avoid-re-export-all
RuleModule-->>ESLint: Reports if export-all syntax is used
ESLint-->>Developer: Displays lint results and warnings/errors
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
WalkthroughThis pull request ports rules from Changes
|
![petercat-assistant[bot]](https://avatars.githubusercontent.com/in/875736?s=60&v=4){kind=small}
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 87277aa in 3 minutes and 33 seconds
More details
- Looked at
1038lines of code in15files - Skipped
1files when reviewing. - Skipped posting
11drafted comments based on config settings.
1. docs/rules/avoid-namespace-import.md:18
- Draft comment:
Incorrect rule key in the example: use 'import-x/avoid-namespace-import' instead of 'import-x/avoid-barrel-files'. - Reason this comment was not posted:
Marked as duplicate.
2. docs/rules/avoid-namespace-import.md:18
- Draft comment:
The default options code block on line 18 references "import-x/avoid-barrel-files", which seems inconsistent with the rule name 'avoid-namespace-import'. If this is a copy-paste error, please update it to "import-x/avoid-namespace-import". - Reason this comment was not posted:
Marked as duplicate.
3. docs/rules/avoid-re-export-all.md:3
- Draft comment:
Consider revising 'treeshaking' to 'tree-shaking' for improved clarity and consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
While 'tree-shaking' might be more commonly written with a hyphen, this is an extremely minor stylistic suggestion. Both versions are widely understood in the JavaScript community. This doesn't impact code functionality or documentation clarity in any meaningful way. The meaning is perfectly clear either way.
The hyphenated version might be more technically correct and consistent with official documentation from bundler tools.
The unhyphenated version is still widely used and perfectly understandable. This is too minor of a change to warrant a PR comment.
Delete this comment as it's an extremely minor stylistic suggestion that doesn't meaningfully improve the documentation.
4. package.json:7
- Draft comment:
The author field currently reads "JounQin". Please confirm if this is the intended spelling or if it should be corrected. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. package.json:15
- Draft comment:
The "types" field is set to "index.d.cts". Typically, TypeScript declaration files use the .d.ts extension. Please verify whether this is intentional (for example, if using .cts for CommonJS types) or if it might be a typo. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. src/index.ts:209
- Draft comment:
In the export statement on line 209 (export type * from './types.js'), it appears that there may be a typographical error. Typically, to re-export all types or values, the syntax used is eitherexport * from './types.js'for re-exporting everything or explicitly listing types (e.g.,export type { SomeType } from './types.js'). Please review and adjust this line to use the correct syntax. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. src/rules/avoid-barrel-files.ts:29
- Draft comment:
Typo: Change 'Minimum amount of exports to consider module as barrelfile' to 'Minimum amount of exports to consider module as barrel file' to improve clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
This is an extremely minor text change that doesn't affect functionality. The term "barrelfile" is used consistently elsewhere, so changing it here would actually reduce consistency. The comment is about pure formatting/style rather than any substantive issue. According to the rules, we should not make purely informative comments or comments about obvious/unimportant things.
Perhaps consistent terminology in documentation is important for user understanding? Maybe having proper spacing in description strings is part of the project's style guide?
While consistency in docs is good, this is too minor to warrant a comment. If there was a style guide rule about this, it should be caught by automated tools rather than manual review.
Delete this comment as it suggests an extremely minor text change that doesn't affect functionality and may actually reduce consistency with how the term is used elsewhere in the code.
8. src/rules/avoid-importing-barrel-files.ts:307
- Draft comment:
Typo: In the comment starting at line 307, "Its not a bare module specifier, but local module, so we need to analyze it" should be corrected to "It's not a bare module specifier, but a local module, so we need to analyze it." - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
While technically correct, comments about grammar in code comments are generally not important enough to warrant a PR comment. The meaning is perfectly clear either way. This is a very minor nitpick that doesn't affect code quality or functionality.
Grammar mistakes in comments could indicate a lack of attention to detail and could make the code less professional. Clear communication is important.
While clear communication is important, this minor grammatical error doesn't meaningfully impact understanding or maintainability. The comment is still perfectly understandable.
This comment should be deleted as it's too minor and doesn't meaningfully improve code quality.
9. src/rules/avoid-importing-barrel-files.ts:54
- Draft comment:
Typo/Consistency: The comment on line 54 reads "custom error class to emulate oxc_resolver ResolveError enum." If the intention is to emulate behavior of the resolver from 'unrs-resolver', consider updating 'oxc_resolver' to 'unrs-resolver' for consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The presence of both 'oxc-resolver' and 'unrs-resolver' in the codebase suggests there may be a relationship between these packages or a transition happening. Without more context about the codebase and these packages, we can't be certain which reference is correct. The comment could be pointing out a real inconsistency, or it could be misunderstanding the codebase's intent.
I might be missing important context about the relationship between oxc_resolver and unrs-resolver. The presence of both terms could be intentional rather than a mistake.
Given the uncertainty and the fact that this is a documentation-only change that requires more context to verify, we should err on the side of not making the suggestion.
The comment should be deleted because it requires more context to verify its correctness, and it's not clearly related to a functional issue in the code.
10. test/rules/avoid-barrel-files.spec.ts:80
- Draft comment:
Typographical error: Consider adding a space before the closing brace in the import statement, i.e., changeimport { bar, baz, qux} from 'foo';toimport { bar, baz, qux } from 'foo';. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
This is a test file where the code in question is actually a string literal used to test the barrel files rule. The spacing issue is extremely minor and doesn't affect the test's functionality. Moreover, since this is inside a template literal used for testing, the exact formatting is not critical as long as the test demonstrates the intended violation of the barrel files rule.
The spacing inconsistency could potentially make the test file less readable or maintainable. Some teams might have strict formatting standards even for test files.
While consistent formatting is good, this is too minor of an issue to warrant a comment, especially in test code that's demonstrating a completely different concern (barrel file detection).
Delete this comment as it's an extremely minor formatting issue in test code that doesn't impact functionality or readability significantly.
11. test/rules/avoid-barrel-files.spec.ts:82
- Draft comment:
Typographical error: The export statement contains an extra trailing comma with extra spacing (export { foo, bar, baz, qux, };). Consider removing the extra comma or adjusting the spacing to clean it up. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
This is a test case that's meant to trigger the "avoid-barrel" rule. The trailing comma and spacing, while technically present, are not relevant to what's being tested. The test is passing as intended - it's detecting a barrel file pattern. The formatting issue doesn't affect the test's functionality or purpose.
The trailing comma could be considered a minor code style issue that should be consistent across the codebase. Maybe inconsistent formatting in tests could be confusing to other developers.
This is a minor formatting issue in a test case that's already working correctly. The comment is focusing on something trivial that doesn't affect the test's purpose or functionality.
Delete this comment. It's pointing out a trivial formatting issue in a test case that's correctly serving its purpose of testing the barrel file detection rule.
Workflow ID: wflow_Oq2GltPVFJytSoAJ
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| type: 'array', | ||
| description: | ||
| 'Export conditions to use to resolve bare module specifiers', | ||
| default: [], |
There was a problem hiding this comment.
Mismatch between JSON schema defaults and actual defaultOptions. The schema sets empty arrays (e.g. for exportConditions, mainFields, extensions) while defaultOptions use specific values.
| default: [], | |
| default: ["node", "import"], |
There was a problem hiding this comment.
we don't do this in other rules, it'd be nice to have this clarified
in other rules, we set defaultOptions to []. and we don't use default. we just setup defaults in create
in this rule, we set defaultOptions and use whatever it is set to in create (but still unsure why we set defaultOptions...)
There was a problem hiding this comment.
but still unsure why we set
defaultOptions...
I believe that's because tseslint complains if we don't?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (10)
docs/rules/avoid-re-export-all.md (2)
1-12: Clear documentation with suggested grammar improvementThe documentation clearly explains the purpose of the rule and provides appropriate examples of incorrect code to avoid.
Consider adding a comma after "module" in line 3 for improved readability:
-This rule forbids exporting `*` from a module as it can lead to unused imports and prevent treeshaking. +This rule forbids exporting `*` from a module, as it can lead to unused imports and prevent treeshaking.🧰 Tools
🪛 LanguageTool
[uncategorized] ~3-~3: Possible missing comma found.
Context: ... This rule forbids exporting*from a module as it can lead to unused imports and pr...(AI_HYDRA_LEO_MISSING_COMMA)
1-12: Documentation could be enhanced with correct examples and optionsWhile the current documentation is concise, it would be helpful to add:
- Examples of correct code
- Documentation for any available options
- A "When Not To Use It" section for cases where re-export-all might be acceptable
Consider enhancing the documentation with additional sections:
Examples of **correct** code for this rule: ```js export { foo, bar } from 'foo' export type * from 'foo' // Type-only exports are allowedOptions
This rule has no options.
When Not To Use It
If you're working in a library where re-exporting all is an intentional design choice and tree-shaking is not a concern.
<details> <summary>🧰 Tools</summary> <details> <summary>🪛 LanguageTool</summary> [uncategorized] ~3-~3: Possible missing comma found. Context: ... This rule forbids exporting `*` from a module as it can lead to unused imports and pr... (AI_HYDRA_LEO_MISSING_COMMA) </details> </details> </blockquote></details> <details> <summary>test/rules/avoid-re-export-all.spec.ts (2)</summary><blockquote> `13-14`: **Consider enabling or removing commented test cases.** There are two commented-out test cases related to TypeScript-specific syntax. These should either be enabled (perhaps with proper TypeScript language options) or removed for clarity if they're not applicable. ```diff - // 'export type { foo } from "foo";', - // 'export type * as foo from "foo";', + { + code: 'export type { foo } from "foo";', + languageOptions: { + parser: require('@typescript-eslint/parser'), + parserOptions: { sourceType: 'module' } + }, + }, + { + code: 'export type * as foo from "foo";', + languageOptions: { + parser: require('@typescript-eslint/parser'), + parserOptions: { sourceType: 'module' } + }, + },Or if these should be tested in a separate TypeScript-specific test file, remove them completely.
9-9: Consider using tValid in addition to tInvalid.You're only destructuring
tInvalidfrom the utility function, but for consistency with how valid test cases are handled, consider usingtValidfor the valid test cases as well.-const { tInvalid } = createRuleTestCaseFunctions<typeof rule>() +const { tValid, tInvalid } = createRuleTestCaseFunctions<typeof rule>()Then use it for valid test cases:
valid: [ - 'export { foo } from "foo";', - 'export { foo as bar } from "foo";', + tValid({ code: 'export { foo } from "foo";' }), + tValid({ code: 'export { foo as bar } from "foo";' }), ],test/rules/avoid-namespace-import.spec.ts (1)
14-15: Consider uncommenting type import testsThese commented-out test cases for type imports would improve test coverage for TypeScript users. Consider including them as active tests to ensure the rule handles type imports correctly.
- // 'import type { foo } from "foo";', - // 'import type * as foo from "foo";' + tValid({ code: 'import type { foo } from "foo";' }), + tValid({ code: 'import type * as foo from "foo";' }),test/rules/avoid-barrel-files.spec.ts (1)
7-8: Consider unifying order of declarationsThe order of declaring
tValid/tInvalidandruleTesteris different from other test files (like avoid-namespace-import.spec.ts). For consistency, consider using the same order in all test files.-const { tValid, tInvalid } = createRuleTestCaseFunctions<typeof rule>() - -const ruleTester = new RuleTester() +const ruleTester = new RuleTester() + +const { tValid, tInvalid } = createRuleTestCaseFunctions<typeof rule>()src/rules/avoid-barrel-files.ts (1)
47-97: Consider adding support for export declarations contained within other declarationsThe current implementation looks at top-level declarations and exports, but doesn't account for exports that might be part of other declarations, such as exports within a namespace or conditional exports. While this covers most common cases, it could be enhanced for completeness.
docs/rules/avoid-importing-barrel-files.md (1)
39-40: Consider improving wordingUsing "Number of exports" instead of "Amount of exports" would be more grammatically correct since exports are countable items.
-Amount of exports to consider a module as a barrel file. +Number of exports to consider a module as a barrel file.🧰 Tools
🪛 LanguageTool
[uncategorized] ~39-~39: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ountOfExportsToConsiderModuleAsBarrel` Amount of exports to consider a module as a ba...(AMOUNTOF_TO_NUMBEROF)
src/rules/avoid-importing-barrel-files.ts (2)
17-18: Reconsider thedebugoption usageThe
debugoption is only used for logging errors during module resolution, which is not typical for ESLint rule options. Consider using ESLint's built-in logging or contextual reporting mechanism instead.
165-174: Simplify options unpackingThe current approach extracts each option individually. Consider using object destructuring for a cleaner implementation.
- const maxModuleGraphSizeAllowed = options.maxModuleGraphSizeAllowed - const debug = options.debug - const amountOfExportsToConsiderModuleAsBarrel = - options.amountOfExportsToConsiderModuleAsBarrel - const exportConditions = options.exportConditions - const mainFields = options.mainFields - const extensions = options.extensions - const tsconfig = options.tsconfig - const alias = options.alias + const { + maxModuleGraphSizeAllowed, + debug, + amountOfExportsToConsiderModuleAsBarrel, + exportConditions, + mainFields, + extensions, + tsconfig, + alias + } = options
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
LICENSE(1 hunks)docs/rules/avoid-barrel-files.md(1 hunks)docs/rules/avoid-importing-barrel-files.md(1 hunks)docs/rules/avoid-namespace-import.md(1 hunks)docs/rules/avoid-re-export-all.md(1 hunks)package.json(1 hunks)src/index.ts(2 hunks)src/rules/avoid-barrel-files.ts(1 hunks)src/rules/avoid-importing-barrel-files.ts(1 hunks)src/rules/avoid-namespace-import.ts(1 hunks)src/rules/avoid-re-export-all.ts(1 hunks)test/rules/avoid-barrel-files-ts.spec.ts(1 hunks)test/rules/avoid-barrel-files.spec.ts(1 hunks)test/rules/avoid-namespace-import.spec.ts(1 hunks)test/rules/avoid-re-export-all.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
test/rules/avoid-barrel-files-ts.spec.ts (1)
test/utils.ts (1)
createRuleTestCaseFunctions(138-153)
test/rules/avoid-barrel-files.spec.ts (1)
test/utils.ts (1)
createRuleTestCaseFunctions(138-153)
test/rules/avoid-re-export-all.spec.ts (1)
test/utils.ts (1)
createRuleTestCaseFunctions(138-153)
src/rules/avoid-barrel-files.ts (5)
src/rules/avoid-namespace-import.ts (2)
Options(3-5)MessageId(7-7)src/rules/avoid-importing-barrel-files.ts (2)
Options(13-26)MessageId(28-28)src/rules/avoid-re-export-all.ts (1)
MessageId(3-3)src/utils/create-rule.ts (1)
createRule(12-12)src/rules/max-dependencies.ts (1)
context(44-58)
src/rules/avoid-re-export-all.ts (1)
src/utils/create-rule.ts (1)
createRule(12-12)
test/rules/avoid-namespace-import.spec.ts (1)
test/utils.ts (1)
createRuleTestCaseFunctions(138-153)
🪛 LanguageTool
docs/rules/avoid-re-export-all.md
[uncategorized] ~3-~3: Possible missing comma found.
Context: ... This rule forbids exporting * from a module as it can lead to unused imports and pr...
(AI_HYDRA_LEO_MISSING_COMMA)
docs/rules/avoid-importing-barrel-files.md
[style] ~3-~3: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...lints against authoring barrel files. This rule lints against importing barrel f...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~39-~39: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ountOfExportsToConsiderModuleAsBarrel` Amount of exports to consider a module as a ba...
(AMOUNTOF_TO_NUMBEROF)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (16)
package.json (1)
79-79:✅ Verification successful
Dependency added for barrel file analysis
The addition of
eslint-barrel-file-utilsdependency supports the new barrel file related rules being ported in this PR. This is appropriate for the functionality described in the PR objectives.
🏁 Script executed:
#!/bin/bash # Check for the latest version of eslint-barrel-file-utils npm view eslint-barrel-file-utils versions --json | jq -r '.[-1]'Length of output: 74
Dependency Version Confirmed
The dependency"eslint-barrel-file-utils": "^0.0.11"inpackage.jsonhas been verified against the latest published version (0.0.11). This correctly supports the barrel file related rules introduced in the PR, and no additional changes are required.src/rules/avoid-namespace-import.ts (3)
1-63: Well-structured rule implementationThe rule implementation follows the expected pattern for ESLint rules with clear options, schema validation, and messaging. The rule correctly identifies namespace imports while providing appropriate exemptions for type-only imports and allowlisted modules.
44-47: Consistent handling of default optionsThe implementation properly handles default options, but I notice that the PR objectives mentioned concerns about the consistent approach with other rules. This implementation looks sound - it uses a dedicated defaultOptions constant and falls back to it when no options are provided.
50-60: Clear rule logic with appropriate exceptionsThe rule correctly exempts both type-only imports and sources that are in the allowList. This prevents false positives while maintaining the performance benefits of avoiding unnecessary namespace imports.
LICENSE (1)
4-4: Copyright notice added appropriatelyThe addition of a copyright notice for "modern-webdev" from 2020 is appropriate since this PR is porting rules from another package (eslint-plugin-barrel-files).
docs/rules/avoid-barrel-files.md (1)
1-28: Documentation looks good and is well-structured.The documentation clearly explains the rule's purpose, provides a relevant example of incorrect code, and properly documents the configuration options. The default threshold of 5 exports to consider a module as a barrel file seems reasonable.
src/index.ts (2)
22-25: LGTM: New rule imports follow existing patterns.The import statements for the new barrel file-related rules follow the established naming pattern used throughout the file.
135-140: LGTM: Rules are properly organized.The new rules are logically grouped under a "barrel files" comment section, making the organization clear and maintainable.
test/rules/avoid-re-export-all.spec.ts (1)
1-30:✅ Verification successful
Verify rule implementation matches test assertions.
The test asserts that both
export * from "foo"andexport * as foo from "foo"should trigger the rule with theavoidReExportmessage ID. Make sure the actual rule implementation matches these assertions.
🏁 Script executed:
#!/bin/bash # Description: Check implementation of avoid-re-export-all rule # 1. Locate and display the rule implementation echo "Looking for the avoid-re-export-all.js rule file..." fd --type file "avoid-re-export-all.js" src # 2. Check messageIds and ESLint selectors in the rule implementation echo "Checking rule implementation for messageIds and node selectors..." grep -A 15 "messageId" src/rules/avoid-re-export-all.js || grep -A 15 "messageId" src/rules/avoid-re-export-all.ts # 3. Check if rule handles both 'export *' and 'export * as' scenarios echo "Checking if rule handles both export patterns..." grep -A 5 "ExportAllDeclaration" src/rules/avoid-re-export-all.js || grep -A 5 "ExportAllDeclaration" src/rules/avoid-re-export-all.tsLength of output: 1042
Verified: The rule logic correctly matches the test assertions.
The implementation in
src/rules/avoid-re-export-all.tsdefines themessageIdasavoidReExportand checksExportAllDeclarationnodes. It reports any re-export that isn’t type-only (i.e. whennode.exportKind !== 'type'), covering both the cases forexport * from "foo";andexport * as foo from "foo";. No discrepancies were observed between the test cases and the rule implementation.src/rules/avoid-re-export-all.ts (1)
1-33: Well-structured and implemented ESLint ruleThis new rule to prevent re-exporting all exports from a module is well-implemented. It correctly identifies
ExportAllDeclarationnodes while allowing type-only re-exports. The rule's message explains the rationale clearly - that re-exporting all exports can lead to unused imports and prevent proper tree-shaking.A few observations:
- The rule has no configurable options, which makes it straightforward but inflexible
- It correctly excludes type exports (with
exportKind === 'type') which is good for TypeScript codebases- It's set as
recommended: true, which makes sense for a performance-focused ruletest/rules/avoid-namespace-import.spec.ts (1)
11-41: Tests cover the rule's behavior wellThe test cases are comprehensive and effectively test both allowed and disallowed namespace imports:
- Line 13: Verifies that named imports are allowed
- Lines 16-23: Confirms that namespace imports in the allowList are permitted
- Lines 27-30: Checks that namespace imports are prohibited by default
- Lines 31-39: Validates that only allowed namespaces (not others) are permitted when using the allowList option
test/rules/avoid-barrel-files-ts.spec.ts (1)
11-61: TypeScript-specific tests are thoroughThe tests comprehensively validate the rule's behavior with TypeScript-specific constructs:
- Lines 13-18: Test simple type exports
- Lines 19-27: Test complex type definitions
- Lines 28-44: Test multiple exports including interfaces, types, and constants
- Lines 48-59: Test detection of barrel-like re-exports involving imported types
This is a good complement to the main test file, ensuring TypeScript-specific scenarios are covered.
test/rules/avoid-barrel-files.spec.ts (1)
11-109: Tests comprehensively cover rule behaviorThe test cases do an excellent job covering various barrel file scenarios:
- Various export counts (lines 13-42)
- Default exports with different configurations (lines 43-74)
- Multiple imports from different sources (lines 78-85)
- Multiple
export * fromstatements (lines 86-94)- Configurable thresholds (lines 95-103)
- Default exports of multiple variables (lines 104-107)
This thorough testing will help ensure the rule functions correctly across different codebases and configurations.
src/rules/avoid-barrel-files.ts (1)
13-41: Rule configuration looks good!The rule definition with options, schema, and default values is well-structured. The documentation in the schema is helpful for users understanding the rule's purpose.
docs/rules/avoid-importing-barrel-files.md (1)
1-3: Documentation title and introduction look good!Clear explanation of the rule's purpose and how it differs from the related
avoid-barrel-filesrule.🧰 Tools
🪛 LanguageTool
[style] ~3-~3: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...lints against authoring barrel files. This rule lints against importing barrel f...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
src/rules/avoid-importing-barrel-files.ts (1)
1-10: Dependencies and imports are appropriateGood use of Node.js built-in modules and external utilities for barrel file detection and module resolution.
| import { | ||
| count_module_graph_size, | ||
| is_barrel_file, | ||
| } from 'eslint-barrel-file-utils/index.cjs' |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify eslint-barrel-file-utils import path
The import uses 'eslint-barrel-file-utils/index.cjs' which might be problematic. According to the PR objectives, there was a missing d.cts file for index.cjs, which could cause typing issues.
🏁 Script executed:
#!/bin/bash
# Check for existence of the d.cts file
echo "Checking for d.cts file for index.cjs in eslint-barrel-file-utils"
ls -la node_modules/eslint-barrel-file-utils/index.d.cts 2>/dev/null || echo "File not found"
# Check the package.json exports configuration
echo "Checking exports configuration in package.json"
cat node_modules/eslint-barrel-file-utils/package.json | grep -A 10 '"exports"'Length of output: 423
Address Missing Type Definitions for eslint-barrel-file-utils Import
- The verification script confirms that the file
node_modules/eslint-barrel-file-utils/index.d.ctsis missing. - Since the PR objectives highlighted a missing
d.ctsfile forindex.cjs, this absence may lead to type issues. - Please either add the missing
index.d.ctsfile to the package or update the import path to a file that properly exposes the required type definitions.
| import { | ||
| count_module_graph_size, | ||
| is_barrel_file, | ||
| } from 'eslint-barrel-file-utils/index.cjs' |
There was a problem hiding this comment.
Hmm, why .cjs is referenced specifically? Without /index.cjs it won't work? Then that's an issue of eslint-barrel-file-utils? I'm wondering should eslint-barrel-file-utils be merged into unrs-resolver at the same time.
There was a problem hiding this comment.
from the OP:
eslint-barrel-file-utils is missing a d.cts for index.cjs, and we can't import index.js (or bare) because the index.d.ts exported names mismatch the ones in index.d.ts
this is why. maybe lets see what @thepassle thinks too
There was a problem hiding this comment.
I'm wondering should it be merged into unrs-resolver instead, at least as a crate dep.
| debug: false, | ||
| exportConditions: ['node', 'import'], | ||
| mainFields: ['module', 'browser', 'main'], | ||
| extensions: ['.js', '.ts', '.tsx', '.jsx', '.json', '.node'], |
There was a problem hiding this comment.
For first version without releasing it is acceptable, but it should be replaced by import-x's own resolver eventually.
There was a problem hiding this comment.
it does use unrs-resolver
can you clarify what you mean?
There was a problem hiding this comment.
It should not call unrs-resolver manually inside, it should call ExportMap.get() to use the resolver concept, so that 3rd-party resolvers can be used.
| type: 'string', | ||
| }, | ||
| }, | ||
| // schema to match oxc-resolver's TsconfigOptions |
There was a problem hiding this comment.
These should really be removed later.
| return | ||
| } | ||
|
|
||
| if (builtinModules.includes(moduleSpecifier.replace('node:', ''))) { |
There was a problem hiding this comment.
Should use the resolver concept and test with resolved.path === null instead.
There was a problem hiding this comment.
how do we know the difference between a path which doesn't exist and a built in?
if i sync(currentFile, 'node:path') i just get a Cannot find module 'node:path'. but we need to handle built-ins differently to missing files
There was a problem hiding this comment.
null means built-in.
https://github.com/un-ts/eslint-plugin-import-x/tree/master/resolvers#example
That's why we should not use unrs-resolver directly in this rule.
|
@43081j By the way, do you want to join this project? |
This ports the rules from `eslint-plugin-barrel-files` into the package.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
src/rules/avoid-importing-barrel-files.ts (6)
8-8: Consider addressing the missing type definitions issue.As noted in previous reviews, the import from
eslint-barrel-file-utils/index.cjsmay cause issues since the PR objectives mentioned a missingd.ctsfile. This import approach might lead to TypeScript typing issues.
108-108: Mismatch between schema defaults and actual defaultOptions.The schema defines empty arrays as defaults while
defaultOptionsuses specific values.- default: [], + default: ["node", "import"],
248-250: Remove redundant error handling code.This error handling appears redundant as the same condition is already handled inside the try block (lines 216-219).
- if (!resolvedPath.path) { - throw new ResolveError('NotFound', resolvedPath.error) - }
275-328: 🛠️ Refactor suggestionExtract duplicate reporting logic into a helper function.
The code has three nearly identical blocks for reporting violations (lines 275-284, 294-303, and 318-327). Following the DRY principle would improve maintainability.
+ function reportBarrelFileViolation(node, moduleSpecifier, moduleGraphSize) { + if (moduleGraphSize > maxModuleGraphSizeAllowed) { + context.report({ + node, + messageId: 'avoidImport', + data: { + amount: moduleGraphSize, + specifier: moduleSpecifier, + maxModuleGraphSizeAllowed, + }, + }); + } + } /** Only cache bare module specifiers, as local files can change */ if (isBareModuleSpecifier(moduleSpecifier)) { /** * The module specifier is not cached yet, so we need to analyze and * cache it */ if (cache[moduleSpecifier] === undefined) { isBarrelFile = is_barrel_file( fileContent, amountOfExportsToConsiderModuleAsBarrel, ) const moduleGraphSize = isBarrelFile ? count_module_graph_size(resolvedPath.path, resolutionOptions) : -1 cache[moduleSpecifier] = { isBarrelFile, moduleGraphSize, } - if (moduleGraphSize > maxModuleGraphSizeAllowed) { - context.report({ - node: node.source, - messageId: 'avoidImport', - data: { - amount: moduleGraphSize, - specifier: moduleSpecifier, - maxModuleGraphSizeAllowed, - }, - }) - } + reportBarrelFileViolation(node.source, moduleSpecifier, moduleGraphSize); } else { /** * It is a bare module specifier, but cached, so we can use the * cached value */ - if ( - cache[moduleSpecifier].moduleGraphSize > maxModuleGraphSizeAllowed - ) { - context.report({ - node: node.source, - messageId: 'avoidImport', - data: { - amount: cache[moduleSpecifier].moduleGraphSize, - specifier: moduleSpecifier, - maxModuleGraphSizeAllowed, - }, - }) - } + reportBarrelFileViolation(node.source, moduleSpecifier, cache[moduleSpecifier].moduleGraphSize); } } else { /** * Its not a bare module specifier, but local module, so we need to * analyze it */ const isBarrelFile = is_barrel_file( fileContent, amountOfExportsToConsiderModuleAsBarrel, ) const moduleGraphSize = isBarrelFile ? count_module_graph_size(resolvedPath.path, resolutionOptions) : -1 - if (moduleGraphSize > maxModuleGraphSizeAllowed) { - context.report({ - node: node.source, - messageId: 'avoidImport', - data: { - amount: moduleGraphSize, - specifier: moduleSpecifier, - maxModuleGraphSizeAllowed, - }, - }) - } + reportBarrelFileViolation(node.source, moduleSpecifier, moduleGraphSize); }
252-253:⚠️ Potential issueFix variable declaration issue.
The
isBarrelFilevariable is declared here without initialization, but then redeclared withconston line 311 in the else block. This can lead to confusion and potential bugs.- let isBarrelFile: boolean + // Variable to hold barrel file detection result + let isBarrelFile = false
52-53:⚠️ Potential issueFix potential null reference when accessing index 0.
The current implementation could throw if
specifieris empty or the replace operation results in an empty string.-const isBareModuleSpecifier = (specifier: string): boolean => - !!specifier?.replace(/'/g, '')[0].match(/[@a-zA-Z]/g) +const isBareModuleSpecifier = (specifier: string): boolean => + !!specifier && !!specifier.replace(/'/g, '')[0]?.match(/[@a-zA-Z]/g)docs/rules/avoid-importing-barrel-files.md (1)
71-72: Incorrect rule name in example.The example configuration uses 'barrel-files/avoid-importing-barrel-files' but the title and rest of the document refer to 'import-x/avoid-importing-barrel-files'.
- 'barrel-files/avoid-importing-barrel-files': [ + 'import-x/avoid-importing-barrel-files': [
🧹 Nitpick comments (5)
src/rules/avoid-importing-barrel-files.ts (1)
164-172: Simplify options extraction using object destructuring.The current approach manually extracts each option individually. Using destructuring would make the code more concise and reduce repetition.
- const options = context.options[0] || defaultOptions - const maxModuleGraphSizeAllowed = options.maxModuleGraphSizeAllowed - const debug = options.debug - const amountOfExportsToConsiderModuleAsBarrel = - options.amountOfExportsToConsiderModuleAsBarrel - const exportConditions = options.exportConditions - const mainFields = options.mainFields - const extensions = options.extensions - const tsconfig = options.tsconfig - const alias = options.alias + const { + maxModuleGraphSizeAllowed, + debug, + amountOfExportsToConsiderModuleAsBarrel, + exportConditions, + mainFields, + extensions, + tsconfig, + alias + } = context.options[0] || defaultOptionsdocs/rules/avoid-importing-barrel-files.md (4)
39-40: Use "Number of" instead of "Amount of" for countable items."Amount of" should typically be used with uncountable or mass nouns. Since exports are countable, "Number of" would be more appropriate.
-Amount of exports to consider a module as a barrel file. +Number of exports to consider a module as a barrel file.🧰 Tools
🪛 LanguageTool
[uncategorized] ~39-~39: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ountOfExportsToConsiderModuleAsBarrel` Amount of exports to consider a module as a ba...(AMOUNTOF_TO_NUMBEROF)
3-3: Improve sentence variety at the beginning of the introduction.Three successive sentences begin with "This rule". Consider rewording one of them to improve readability.
-This rule aims to avoid importing barrel files that lead to loading large module graphs. This rule is different from the `avoid-barrel-files` rule, which lints against _authoring_ barrel files. This rule lints against _importing_ barrel files. +This rule aims to avoid importing barrel files that lead to loading large module graphs. It differs from the `avoid-barrel-files` rule, which lints against _authoring_ barrel files. Specifically, this rule lints against _importing_ barrel files.🧰 Tools
🪛 LanguageTool
[style] ~3-~3: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...lints against authoring barrel files. This rule lints against importing barrel f...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
22-22: Update parameter name for consistency.To ensure consistency with the documentation suggestion above, consider updating the parameter name in the example configuration.
- amountOfExportsToConsiderModuleAsBarrel: 3, + numberOfExportsToConsiderModuleAsBarrel: 3,This would also need to be updated in the implementation file.
36-36: Add clarification about module graph size calculation.The documentation explains what the rule does but doesn't provide details on how the module graph size is calculated. This would be helpful for users trying to understand why certain imports are flagged.
Add the following explanation:
-Maximum allowed module graph size. If an imported module results in loading more than this many modules, it will be considered a barrel file. +Maximum allowed module graph size. If an imported module results in loading more than this many modules, it will be considered a barrel file. The module graph size is calculated by counting all unique modules that are imported directly or indirectly by the barrel file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
LICENSE(1 hunks)docs/rules/avoid-barrel-files.md(1 hunks)docs/rules/avoid-importing-barrel-files.md(1 hunks)docs/rules/avoid-namespace-import.md(1 hunks)docs/rules/avoid-re-export-all.md(1 hunks)package.json(1 hunks)src/index.ts(2 hunks)src/rules/avoid-barrel-files.ts(1 hunks)src/rules/avoid-importing-barrel-files.ts(1 hunks)src/rules/avoid-namespace-import.ts(1 hunks)src/rules/avoid-re-export-all.ts(1 hunks)test/rules/avoid-barrel-files-ts.spec.ts(1 hunks)test/rules/avoid-barrel-files.spec.ts(1 hunks)test/rules/avoid-namespace-import.spec.ts(1 hunks)test/rules/avoid-re-export-all.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- LICENSE
- docs/rules/avoid-re-export-all.md
- test/rules/avoid-namespace-import.spec.ts
- docs/rules/avoid-namespace-import.md
- docs/rules/avoid-barrel-files.md
- test/rules/avoid-re-export-all.spec.ts
- src/rules/avoid-re-export-all.ts
- package.json
- test/rules/avoid-barrel-files.spec.ts
- test/rules/avoid-barrel-files-ts.spec.ts
- src/index.ts
- src/rules/avoid-barrel-files.ts
- src/rules/avoid-namespace-import.ts
🧰 Additional context used
🪛 LanguageTool
docs/rules/avoid-importing-barrel-files.md
[style] ~3-~3: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...lints against authoring barrel files. This rule lints against importing barrel f...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~39-~39: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ountOfExportsToConsiderModuleAsBarrel` Amount of exports to consider a module as a ba...
(AMOUNTOF_TO_NUMBEROF)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
sure i'm happy to help where i can 👍 i've updated the branch. sorry it took so long, i got distracted by a bunch of things 😅 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
docs/rules/avoid-importing-barrel-files.md (5)
1-4: Reduce Repetition in Introductory Sentences
The first three sentences begin with “This rule…”, which can feel repetitive. Consider merging or rephrasing to improve readability.🧰 Tools
🪛 LanguageTool
[style] ~3-~3: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...lints against authoring barrel files. This rule lints against importing barrel f...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
7-13: Add a Correct Usage Example
Currently only an incorrect import example is shown. Adding a corresponding correct code snippet (e.g., importing directly from a module that doesn’t create a large graph) will help users understand the expected usage.
30-33: ClarifyallowListDescription
Consider rephrasing for clarity, e.g.:
“List of module specifiers for which barrel file imports are permitted.”
38-41: Use “Number” Instead of “Amount”
In “Amount of exports to consider a module as a barrel file.”, replace “Amount of” with “Number of” to accurately describe countable exports.🧰 Tools
🪛 LanguageTool
[uncategorized] ~39-~39: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ountOfExportsToConsiderModuleAsBarrel` Amount of exports to consider a module as a ba...(AMOUNTOF_TO_NUMBEROF)
82-83: Remove Stray Code Fence
There appears to be an extra closing triple-backtick at the end of the file—please remove it to avoid rendering issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/rules/avoid-importing-barrel-files.md(1 hunks)docs/rules/avoid-namespace-import.md(1 hunks)src/rules/avoid-importing-barrel-files.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/rules/avoid-namespace-import.md
- src/rules/avoid-importing-barrel-files.ts
🧰 Additional context used
🪛 LanguageTool
docs/rules/avoid-importing-barrel-files.md
[style] ~3-~3: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...lints against authoring barrel files. This rule lints against importing barrel f...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~39-~39: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ountOfExportsToConsiderModuleAsBarrel` Amount of exports to consider a module as a ba...
(AMOUNTOF_TO_NUMBEROF)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (7)
docs/rules/avoid-importing-barrel-files.md (7)
14-28: Options Section Looks Good
The## Optionsheader and default configuration snippet clearly document available settings and defaults.
34-37: ApprovemaxModuleGraphSizeAllowedExplanation
The description is concise and clearly communicates the threshold behavior.
42-45: ApproveexportConditionsDescription
The explanation of export conditions is precise and aligns with resolver options.
46-49: ApprovemainFieldsDescription
The documentation clearly lists the resolution order via mainFields.
50-53: ApproveextensionsDescription
The extensions list is exhaustive and documented clearly.
54-59: ApprovetsconfigSection
The purpose and use-case of thetsconfigoption are well explained.
60-63: ApprovealiasDescription
The mapping to Webpack’s resolve.alias is clearly documented.
|
Just sent the invitation, welcome!
Hmm... CI is still broken unfortunately. 🥹 |
|
Yes it is, because of the comment in the op:
|
Can we use one of the following strategies temporarily:
|
|
We should be able to talk to @thepassle and just fix the type definitions I think We don't need to work around it. He will be happy to help I'm sure |
Sure, those workarouds can be used on current prototype, the CI error is just, annoying. |
|
Tests pass locally. We just need to update the type definitions in the upstream repo Patience :D it'll be fine When I'm at a laptop again I can open a pr |
|
|
It could be. Would be good to see what @thepassle thinks too but that sounds like a sensible idea to me It uses oxc resolver under the hood too I think. So makes sense to get it to use the same resolver as this plugin itself |
This ports the rules from
eslint-plugin-barrel-filesinto the package.cc @thepassle @JounQin
TODO
eslint-barrel-file-utilsis missing ad.ctsforindex.cjs, and we can't importindex.js(or bare) because theindex.d.tsexported names mismatch the ones inindex.d.tsdebugoption ofavoid-importing-barrel-filesdefaultOptionsbut maybe we should use[]and type check everything like the other rules? or maybe the other rules should be more like this?update:eslint-docsdidn't change anything for meImportant
Ported rules from
eslint-plugin-barrel-filesinto the package, adding new rules, documentation, and tests for handling barrel files, namespace imports, and re-exports.avoid-barrel-files,avoid-importing-barrel-files,avoid-namespace-import, andavoid-re-export-allrules fromeslint-plugin-barrel-files.avoid-barrel-files.ts,avoid-importing-barrel-files.ts,avoid-namespace-import.ts, andavoid-re-export-all.ts.docs/rules/.test/rules/.eslint-barrel-file-utilstodependenciesinpackage.json.index.tsto include new rules.This description was created by
for 87277aa. It will automatically update as commits are pushed.
Summary by CodeRabbit