Skip to content

fix: rule cjs-require ignore problem#1613

Open
yifancong wants to merge 2 commits intomainfrom
fix/cjs-require-problem
Open

fix: rule cjs-require ignore problem#1613
yifancong wants to merge 2 commits intomainfrom
fix/cjs-require-problem

Conversation

@yifancong
Copy link
Copy Markdown
Contributor

@yifancong yifancong commented Apr 8, 2026

Summary

This pull request updates the cjs-require rule to improve configurability and simplify the logic for ignoring certain module paths. The main changes include defaulting the ignore configuration to ['node_modules'], removing the hardcoded check for node_modules paths, and updating documentation for clarity.

Configuration improvements:

  • The default value for the ignore option in the rule configuration is now ['node_modules'], making it easier to customize which module paths are ignored without modifying code.
  • The Config interface documentation in types.ts has been updated to clarify that ignore defaults to ['node_modules'].

Related Links

Copilot AI review requested due to automatic review settings April 8, 2026 11:07
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the cjs-require rule’s configuration defaults/documentation so node_modules can be ignored via config (instead of a hardcoded path check), aiming to address “ignore” handling problems for this rule.

Changes:

  • Set cjs-require rule defaultConfig.ignore to ['node_modules'].
  • Remove the hardcoded isNodeModulesPath() issuer skip logic.
  • Update Config.ignore docstring to describe the new default.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
packages/core/src/rules/rules/cjs-require/types.ts Documents that ignore defaults to ['node_modules'].
packages/core/src/rules/rules/cjs-require/index.ts Changes default ignore config and removes the prior issuer-only node_modules bypass.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 16 to 20
category: 'bundle',
severity: Linter.Severity.Warn,
defaultConfig: {
ignore: [],
ignore: ['node_modules'],
},
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting defaultConfig.ignore to ['node_modules'] will cause this rule to skip any dependency where either the issuer path or the required module path contains node_modules (see the ignore check below). Since requiredModule.path is typically inside node_modules for third-party requires, this likely suppresses the primary case this rule is meant to report (user code calling require('pkg')). Consider reverting the default to [] and keeping an explicit (cross-platform) “issuer is in node_modules” skip (e.g. the /[/\]node_modules[/\]/ pattern used in esm-resolved-to-cjs), or split config so only issuers are ignored by default.

Copilot uses AI. Check for mistakes.
@yifancong yifancong requested review from fansenze and fi3ework April 9, 2026 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants