Skip to content
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

feat: add restrictedNamedExportsPattern to no-restricted-exports #18431

Merged

Conversation

akulsr0
Copy link
Contributor

@akulsr0 akulsr0 commented May 8, 2024

…ed-exports`

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[x] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Made changes to add support for #18360

Is there anything you'd like reviewers to focus on?

I feel there should be alot more test cases which needs to be added, can add that based on review insights.

@akulsr0 akulsr0 requested a review from a team as a code owner May 8, 2024 06:58
@eslint-github-bot eslint-github-bot bot added the bug ESLint is working incorrectly label May 8, 2024
Copy link

linux-foundation-easycla bot commented May 8, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@github-actions github-actions bot added the rule Relates to ESLint's core rules label May 8, 2024
Copy link

netlify bot commented May 8, 2024

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit 9e06882
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/664210554f28130008b5497c

@Tanujkanti4441
Copy link
Contributor

Hi @akulsr0, thanks for the PR, but the accepted proposal was to add a new option name restrictedNamedExportsPattern that will be a string containing regex pattern to specify the exports to restrict. Can you do the same?

@akulsr0
Copy link
Contributor Author

akulsr0 commented May 8, 2024

Hi @Tanujkanti4441, it seems having restrictedNamedExportsPattern and restrictedNamedExports both would be repetitive. Since, we can do check for regular strings and string patterns with one single option, added that way.

@Tanujkanti4441
Copy link
Contributor

Hi @Tanujkanti4441, it seems having restrictedNamedExportsPattern and restrictedNamedExports both would be repetitive. Since, we can do check for regular strings and string patterns with one single option, added that way.

since something like this is already done in no-restricted-imports rule so i think it's fine to have both the options.

@mdjermanovic mdjermanovic added the accepted There is consensus among the team that this change meets the criteria for inclusion label May 8, 2024
@mdjermanovic mdjermanovic changed the title fix: allow glob patterns for restrictedNamedExports in `no-restrict… feat: add restrictedNamedExportsPattern to no-restricted-exports May 8, 2024
@eslint-github-bot eslint-github-bot bot added the feature This change adds a new feature to ESLint label May 8, 2024
@mdjermanovic mdjermanovic removed the bug ESLint is working incorrectly label May 8, 2024
lib/rules/no-restricted-exports.js Outdated Show resolved Hide resolved
lib/rules/no-restricted-exports.js Outdated Show resolved Hide resolved
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

Please be sure to update the documentation as well.

lib/rules/no-restricted-exports.js Outdated Show resolved Hide resolved
docs/src/rules/no-restricted-exports.md Show resolved Hide resolved
docs/src/rules/no-restricted-exports.md Show resolved Hide resolved
tests/lib/rules/no-restricted-exports.js Outdated Show resolved Hide resolved
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

Looks good, just we didn't discuss how this option would correlate with restrictDefaultExports. For example, when the pattern matches "default" but some default exports are explicitly allowed by restrictDefaultExports.

I think the best solution is that this option simply doesn't apply to default exports.

docs/src/rules/no-restricted-exports.md Outdated Show resolved Hide resolved
docs/src/rules/no-restricted-exports.md Show resolved Hide resolved
lib/rules/no-restricted-exports.js Outdated Show resolved Hide resolved
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

A few more tests to confirm that this option doesn't apply to default.

tests/lib/rules/no-restricted-exports.js Show resolved Hide resolved
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Leaving open for @Tanujkanti4441 to verify.

Copy link
Contributor

@Tanujkanti4441 Tanujkanti4441 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for contributing!

@mdjermanovic mdjermanovic merged commit b67eba4 into eslint:main May 14, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion contributor pool feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

None yet

5 participants