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 rule: no restricted export names #9180
Conversation
Thanks for the pull request, @bmeck! I took a look to make sure it's ready for merging and found some changes are needed:
Can you please update the pull request to address these? (More information can be found in our pull request guide.) |
|
||
create(context) { | ||
const sourceCode = context.getSourceCode(); | ||
const names = new Set([ |
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.
How about adding the full set of Object.prototype members? I doubt that any good can come from silently allowing them.
"__proto__",
"__defineGetter__",
"__defineSetter__",
"__lookupGetter__",
"__lookupSetter__",
"hasOwnProperty",
"isPrototypeOf",
"propertyIsEnumerable",
"toLocaleString"
(edited by @not-an-aardvark to prevent __text__
from being interpreted as markdown formatting)
5943f65
to
090a913
Compare
Thanks for the pull request, @bmeck! I took a look to make sure it's ready for merging and found some changes are needed:
Can you please update the pull request to address these? (More information can be found in our pull request guide.) |
090a913
to
0615ce3
Compare
LGTM |
Thanks for the PR. I think this seems like a good idea, although I'm unsure if having a hardcoded list of forbidden exports is the best way to do this. For example, I can imagine legitimate uses for exporting export function hasOwnProperty(object, key) {
return Object.prototype.hasOwnProperty.call(object, key);
} I can see how exporting |
Many lint rules have exceptions, this is no different and so I won't try to argue that. These names set forth expectations generally when found on objects coming from JS spec APIs. In particular, the API of those functions are generally expected to be using the export function hasOwnProperty(key) {
return Object.prototype.hasOwnProperty.call(this, key);
} so that it conforms to the expectations set forth by If you can think of a reason the common case should be to allow these, that is fine. However, changing the API of the function names in question makes me still think these should be considered a linting issue. |
Thank you for this proposal and implementing. I have understood that |
@bmeck thanks for the pull request, this looks interesting. You skipped over the question asking you to describe the purpose and function of the rule, which I don't think is particularly clear (as I think is apparent from the team's responses). Can you go back and describe in a paragraph or two what it is you are trying to accomplish with this rule? Also note that we do require documentation to be submitted with new rule proposals. Thanks! |
I'm not sure it's a good idea, since we only support ES standard -- |
I'm generally on board with this idea, but I think it would make the most sense to have no default list and just allow users to specify invalid exports. Of course, we could have some convenience options (like That said, is there any risk for export names outside of dynamic import? I'm a bit confused on that. If the only risk is in dynamic import, then I don't think we should accept this proposal until dynamic import hits stage 4. |
Thanks for your interest in improving ESLint. Unfortunately, it looks like this issue didn't get consensus from the team, so I'm closing it. We define consensus as having three 👍s from team members, as well as a team member willing to champion the proposal. This is a high bar by design -- we can't realistically accept and maintain every feature request in the long term, so we only accept feature requests which are useful enough that there is consensus among the team that they're worth adding. Since ESLint is pluggable and can load custom rules at runtime, the lack of consensus among the ESLint team doesn't need to be a blocker for you using this in your project, if you'd find it useful. It just means that you would need to implement the rule yourself, rather than using a bundled rule that is packaged with ESLint. |
Given that |
After In either case, I think the proposal will probably get more attention if a new issue is opened for it. |
@not-an-aardvark the same hazard applies now for I can certainly open another issue if you think that it has a chance of being accepted. |
Sure, let's discuss it on another issue. |
Filed #10428 |
What is the purpose of this pull request? (put an "X" next to item)
[ ] Documentation update
[ ] Bug fix (template)
[x] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
Please describe what the rule should do:
What category of rule is this? (place an "X" next to just one item)
[ ] Enforces code style
[x] Warns about a potential error
[ ] Suggests an alternate way of doing something
[ ] Other (please specify:)
Provide 2-3 code examples that this rule will warn about:
Why should this rule be included in ESLint (instead of a plugin)?
These names can have introduce surprising effects like some described in tc39/proposal-dynamic-import#48
What changes did you make? (Give an overview)
Added a rule named
no-restricted-exports
and test.Is there anything you'd like reviewers to focus on?