-
-
Notifications
You must be signed in to change notification settings - Fork 55
fix(no-restricted-require): Handle .. paths #458
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
fix(no-restricted-require): Handle .. paths #458
Conversation
82d1c18 to
1e6a4f7
Compare
|
Note: The failure in "CI / Lint (pull_request)" (i.e., https://github.com/eslint-community/eslint-plugin-n/actions/runs/16524228429/job/46733273822?pr=458) seems to be unrelated to this PR. Update: |
1658957 to
b160a35
Compare
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.
Pull Request Overview
This PR fixes the no-restricted-require rule to properly handle .. paths in glob patterns by replacing Minimatch with globrex for pattern matching. This aligns the rule's behavior with ESLint's deprecated no-restricted-modules rule, ensuring that patterns like **/foo correctly match require statements with relative paths containing ...
Key changes:
- Replaces Minimatch with globrex for glob pattern matching
- Adds test case to verify
**/foopattern matches../../foorequire statement - Updates dependencies to include globrex and its TypeScript types
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/util/check-restricted.js | Replaces Minimatch with globrex for pattern matching logic |
| package.json | Adds globrex and @types/globrex dependencies |
| tests/lib/rules/no-restricted-require.js | Adds test case for .. path matching with glob patterns |
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.
LGTM, thanks! Would like @scagood to take a look before merging.
b160a35 to
8a91cfe
Compare
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.
I approve in general, having read through some of the tests in globrex I do wonder if this is a breaking change?
Personally I think we should enable globstar.
Do you think we should also replace the minimatch with globrex at
lib/util/get-convert-path.js:7?
no-restricted-require uses Minimatch to match the given names from the rule's configuration to the require statements it evaluates. As stated in eslint-community#151, this behavior does not match `..` to the `**` glob pattern. This is considered as intended behavior from Minimatch (see isaacs/minimatch#230), but also means that the rule's behavior differs from eslint's deprecated no-restricted-modules rule it's supposed to replace. Using [`globrex`](https://www.npmjs.com/package/globrex) can fix this behavior. Fixes eslint-community#151
8a91cfe to
2b925cd
Compare
@scagood Shoot - you're right.
I think we should - I actually mentioned it at the top of the PR:
I just wanted to see if this PR gets a consensus around it before piling on more work. |
Follow up on eslint-community#458 - as suggested in the PR discussion, replace minimatch with globrex across the board, so we ship only one pattern matching dependency.
no-restricted-requireuses Minimatch to match the given names from the rule's configuration to the require statements it evaluates.As stated in #151, this behavior does not match
..to the**glob pattern. This is considered as intended behavior from Minimatch (see isaacs/minimatch#230), but also means that the rule's behavior differs from eslint's deprecated no-restricted-modulesrule it's supposed to replace.Using
globrexcan fix this behavior.Side notes:
get-convert-path.js. If this PR is accepted, maybe's it's worth considering changing that one too, so there's only only glob-handling dependency to rely on ("worth" as in "I can contribute this change if everyone think this is a good idea")Fixes #151