New rule: no-restricted-exports

[ljharb]

(Continuation of #9180)

Please describe what the rule should do:
Currently, if you get an ES Module namespace object with import * as foo from 'path', there's a hazard: that module might have a named function export of "then", which means Promise.resolve(foo) or await foo won't do what you expect.

This is also particularly hazardous in the current stage 3 import() proposal, so the need will increase in the future - but the need is present now, based on ES2015.

I'd like to provide a rule that defaults to blocking named exports named "then", but could also be configured for any arbitrary list of restricted export names (including "default").

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:

export function then() {
  return "foo";
}

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

Why should this rule be included in ESLint (instead of a plugin)?
Module namespace objects are frozen - they're supposed to conceptually represent a static, pre-runtime-determined, representation of an ES Module. It's very very surprising that a module could suddenly become thenable, breaking its consumers, or suddenly become not thenable, also breaking its consumers.

This rule will help prevent problematic exports in the first place by warning when they are created.

[ljharb]

cc @bmeck @devsnek

[not-an-aardvark]

Thanks for creating another issue. I have a few questions:

• Wouldn't the hazard with import * as foo from 'path'; Promise.resolve(foo)also apply to any other object with an unexpected then method? To me, it doesn't seem specific to exported functions. For example, the following examples seem equally hazardous to me in the absence of import() syntax:

  import * as foo from 'path';
  
  Promise.resolve(foo);

  const foo = getSomeObject();
  
  Promise.resolve(foo);

  I could potentially see an argument for disallowing all functions called then regardless of whether they're exported (although this would have false positives for developers that intentionally implement thenables).

• Could you elaborate on why you want to disallow other export names such as default? My concern here is that the reasons for which a user might want to disallow exporting then (it causes surprising behavior with import() ) are orthogonal to the reasons a user might want to disallow other export names (surprising behavior that would apply to any object, consistency, aesthetic preference, etc.) As a result, I wonder if it would be better to have a dedicated rule for the then case, and leave the other cases to no-restricted-syntax or a similar rule.

[ljharb]

Yes, it definitely would apply to all objects - but Module Namespace objects are special, because as explained in the OP, they conceptually represent a static, frozen, representation of a module. I think it was a mistake to allow them to be thenable, and had TC39 thought about it before it was web-incompatible, we would have found an alternative solution.

I don't think leaving anything to no-restricted-syntax is a good idea; having individual rules able to have their own documentation, and be independently configured and enabled/disabled, is immensely valuable.

As to your question, i think it's a good idea to make all things configurable, especially when the implementation to do so is as trivial as it would be here - but if the consensus was to start out with no configuration, and only deal with then, that'd be fine too.

[not-an-aardvark]

I'm not entirely convinced. If I use import * as foo from 'bar'; Promise.resolve(foo), then foo is still a static, frozen representation of a module -- it's just that Promise.resolve means something more complicated than "create a Promise that fulfills with the given value". The mistaken assumption in that code is in the use of Promise.resolve, not in the module object itself. import('bar') is different because the user never has access to the module namespace object in the first place, and we're in agreement that it's confusing for an expression like import('bar') to return something other than a promise for a module namespace object.

My concern about adding options here is that it could result in user confusion about the purpose of the rule. Effectively, the rule would be filling two different use cases: preventing then to avoid bugs, and preventing other configured names for consistency/aesthetics/etc. As a result, a user might end up mistakenly disabling the check for then because they misunderstand its importance, or (if we made it hard to accidentally disable) they might be confused about why a name was being disallowed if they weren't explicitly configuring it. We could use different error messages/documentation for then and for names configured elsewhere to clarify the problem, but at that point the complexity would be roughly the same as making two separate rules anyway.

In any case, I don't feel strongly about this point, so I'm open to adding options if other team members are in favor of the idea.

[bmeck]

I'm fine with a rule just for then and a different one or follow on for other names/configuration. Per @ljharb 's points, then is a refactoring hazard for consumers moving to/from static/dynamic import.

// x
export function then() {}

let ns = await import('x'); // never resolves

import * as ns 'x'; // resolves

In particular having a rule to prevent behavior for then on export 'd names gives codebases some better guarantees about refactoring to/from static/dynamic import. This is not true for the other names, which are centered around treating Module Namespace Objects as exotic objects to avoid accidental changes to operators. import isn't the same as other operators since it is related / functions according to code outside of the current source text.

[eslint-deprecated]

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

[ljharb]

This is still a concern, and it'd be great to reopen this. (this is also the second related issue that's been auto-closed)

[ljharb]

While I think no-restricted-exports is a more general approach, a specific rule for prohibiting a then export is totally fine with me, as that's really the important bit (for TC39 as well).

Would someone be willing to champion a no-export-then rule? If so, I can update the OP to reflect that.

[platinumazure]

I don't see why we couldn't do no-restricted-exports. I will champion this rule proposal.

[not-an-aardvark]

I'm fine with either no-restricted-exports or a more specific rule about exporting then.

[kaicataldo]

This has now been accepted.

[mdjermanovic]

I could work on this.

Is the decision to implement one generic rule with pre-populated "then" final? That looks confusing, I agree with this comment by @not-an-aardvark.

[platinumazure]

@mdjermanovic The proposal I had championed was a full implementation of no-restricted-exports, though I can't speak to whether everyone who 👍'd the issue was thinking the same thing.

[ljharb]

I’d prefer the full implementation, fwiw.

[mdjermanovic]

What should be the configuration for the rule? A list that defaults to ["then"] looks like an obvious choice, but it means that any user-defined list would overwrite "then" if it isn't explicitly included.

Another choice is a boolean option for "then" only (e.g., restrictThen, default true) and a separate array of strings (e.g., additionalNames, default []) for an arbitrary list? "then" likely needs a detailed explanation in the docs anyway.

(I'd vote for two separate rules if it isn't too late for that discussion)

[ljharb]

I think the general rule is more pressing; i think a separate rule should be proposed for “then” (which can have appropriate docs and messaging). Two rules in the long run is ideal.

[platinumazure]

@mdjermanovic I would suggest empty list by default, similar to no-restricted-syntax and similar rules.

[mdjermanovic]

Okay, I definitely didn't understand what is accepted, since the proposal was 'defaults to blocking named exports named "then"'.

So, the no-restricted-exports rule restricts nothing by default and has nothing about the "then" issue in the documentation.

I'm working on this, should be ready soon.

[ljharb]

yay!

[l1bbcsg]

I'd like to point out that confusion about rule purpose that was mentioned in this issue is not really resolved.
When I discovered this rule was added and read through its docs I was completely lost as to why should I or anyone else impose arbitrary restriction on variable names in exports and why this rule is just a slightly more specific version of id-blacklist.
Note how id-blacklist mentions some sane settings for itself, but no-restricted-exports only mentions foo and a, I had to search for this issue to figure out why this was even introduced.
Perhaps mention of then or other possible application for this rule should be added to docs in text form.

[ljharb]

That sounds like a great addition.