-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
New: Add no-restricted-exports rule (fixes #10428) #12546
Conversation
The code looks fine, and I think all of the assumptions are valid. However, I notice more and more people do not want to use default exports, because named exports are a lot more descriptive. It would be nice to add an option to this rule to disallow default exports. But that could be done in a separate PR, I guess. |
👍 to add the option in a separate PR. Though, it might be good to already think about how the option fits the current design.
The new boolean option would disallow just For instance,
|
While Alternatively, could we replace the first configuration option with an object and do something like the following? ["error", {
restrictedExports: ["default"],
disallowExportDefault: true
}] |
This looks better to me! Maybe Given that is seems (based on votes) that the new option will be added at some point, I'm 👍 to change the initial configuration now. @ilyavolodin would you agree with this change? (asking because you already approved the current version) |
Sounds good to me 👍 |
Configuration is now changed from
(if that isn't okay, I'll revert the change) |
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 for the comprehensive test suite.
What is the purpose of this pull request? (put an "X" next to item)
[X] New rule #10428
Examples of incorrect code for this rule:
What changes did you make? (Give an overview)
New rule
no-restricted-exports
Is there anything you'd like reviewers to focus on?
Configuration is
["error", ["foo", "bar"]]
instead of["error", "foo", "bar"]
This might be a better choice as it's clear that
"error"
isn't a restricted name, but it's different from other similar rules. Is it okay? There was a discussion about this in #11331.This wasn't mentioned in the issue thread, should the rule support custom messages for configured restricted names?
The rule always reports
Identifier
nodes, rather than the declaration nodes (because a declaration can export more names, including the valid ones).Configured
"default"
doesn't disallowexport default
(although it does export"default"
name). Is it okay? There is a section in the documentation about this.This can export restricted names, but it's out of scope for this rule and noted in the Known Limitations section.
I'm not sure what to note as a typical use case in the very first section (maybe nothing?).