new rule: no restricted export names

[bmeck]

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:

<!-- put your code examples here -->
export function then() {
  return "foo";
}

export {toString} from "./x.json";

function v() {return 123;}
export {v as valueOf};

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?

[eslintbot]

Thanks for the pull request, @bmeck! I took a look to make sure it's ready for merging and found some changes are needed:

• The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

[jsf-clabot]

All committers have signed the CLA.

[robpalme]

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)

[eslintbot]

Thanks for the pull request, @bmeck! I took a look to make sure it's ready for merging and found some changes are needed:

• The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

[eslintbot]

LGTM

[not-an-aardvark]

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 hasOwnProperty as a utility method:

export function hasOwnProperty(object, key) {
  return Object.prototype.hasOwnProperty.call(object, key);
}

I can see how exporting then would be very confusing when using import(), because (if I'm understanding correctly) it actually changes the value of the object that you receive when you call import() on the module. I'm less sure about whether the other names here are error-prone in the same way. I think users shouldn't rely on accessing Object.prototype methods from objects they don't own in general (which can be prevented by no-prototype-builtins), but assuming that importers don't rely on that, I don't see anything wrong with exporting a function called e.g. toString.

[bmeck]

@not-an-aardvark

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 this value when called. I cannot think of a good reason to have:

export function hasOwnProperty(key) {
  return Object.prototype.hasOwnProperty.call(this, key);
}

so that it conforms to the expectations set forth by Object.prototype.

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.

[mysticatea]

Thank you for this proposal and implementing.

I have understood that export function then() makes super surprising. However, I'm not sure why we should disallow other names. Could you clarify?

[nzakas]

@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!

[aladdin-add]

These names can have introduce surprising effects like some described in tc39/proposal-dynamic-import#48

I'm not sure it's a good idea, since we only support ES standard -- dynamic-import is a stage-3 feature.

[platinumazure]

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 disallowObjectPrototypeProperties: true would add anything on Object.prototype).

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.

[not-an-aardvark]

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.

[ljharb]

Given that import() gets super weird when a module has a named then export, and the recent TC39 reaction to the Symbol.thenable proposal was that this would continue to be allowed, could we reopen and revisit this?

[not-an-aardvark]

After import() reaches stage 4, I'd champion a rule that disallows exporting a function called then. I'm not convinced we should disallow exported functions with names from Object.prototype, as suggested in this PR.

In either case, I think the proposal will probably get more attention if a new issue is opened for it.

[ljharb]

@not-an-aardvark the same hazard applies now for import * as foo from 'path'; Promise.resolve(foo) // or: await foo, so I think it shouldn't have to wait for stage 4.

I can certainly open another issue if you think that it has a chance of being accepted.

[not-an-aardvark]

Sure, let's discuss it on another issue.

[ljharb]

Filed #10428