Update: Restrict re-exports in no-restricted-imports

[j-f1]

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

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

Fixes #9678.

What changes did you make? (Give an overview)

I added checks for the ExportNamedDeclaration and ExportAllDeclaration nodes to no-restricted-exports

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

Are there any tests I should add?

[ilyavolodin]

Looks like there's some test duplicates, and build is failing because of that.

[ljharb]

Can this be added behind an option? Otherwise it'll need to be semver major, since it causes new warnings to be issued.

[not-an-aardvark]

According to our semver policy, a bugfix which causes new warnings to be reported is semver-minor.

[ljharb]

How is it a bug fix?

I agree that conceptually a re-export is an import + an export, and that this rule should cover it, but the rule is called “imports” and there’s no import in the code this PR warns on. It’s a new feature imo.

[not-an-aardvark]

In my opinion, it's a bug fix because based on the description of the rule, I think a user would expect the rule to already be enforcing this case. As a result, the fact that the rule does not currently enforce this case would be considered a defect.

It's true that this code pattern does not contain import, but I think the rule name refers to the conceptual notion of importing a module (which is happening in this case via the import + re-export that you described), regardless of whether the import keyword is used.

[j-f1]

Without this PR, you could avoid this being blocked:

import fs from 'fs'

By using this code:

// utils.js
export { default as somethingInnocuous } from 'fs'

// example.js
import { somethingInnocuous as fs } from './utils'

[ljharb]

@j-f1 yes that's the behavior i want by default. I want to choose to enforce it more strongly via an option.

[platinumazure]

Is there any strong reason not to add an option?

This does seem like a slightly unusual case. Do we have evidence that a lot of people would "abuse" the current "loophole", and would that justify making this a default behavior change?

[not-an-aardvark]

@ljharb Could you elaborate on why you want the current behavior? I'm having trouble understanding why the current behavior would be useful in comparison to the modified behavior.

[ljharb]

@not-an-aardvark because eslint's semver policy is not as strict as the one I'd want to enforce downstream of my shared config; any additional warnings are semver-major, for me. Without an option, I have no way of controlling this behavior (separate from whatever the default is).

[not-an-aardvark]

It seems like anyone using eslint-config-airbnb would be depending on eslint directly anyway, so it would be the user's responsibility to ensure that they pin the version appropriately for their project. This would be the case regardless of what shareable configs are being used along with eslint.

In order to fulfill the requirement that all users of eslint-config-airbnb can use ^ for their eslint version without fear of broken builds, it seems like eslint would have to add an option for every bugfix that increases the number of warnings. This seems onerous (we have a lot of bugfixes like that), and it would effectively defeat the point of our semver policy, because we wouldn't be able to change the existing behavior without a major release. That's why we tell people to use ~ for their eslint version if they want to avoid broken builds.

[ljharb]

That's not what I'm talking about; I'm saying that I want (not actually talking about the airbnb config, but let's say that one) to be able to upgrade eslint (even if users are using ~) without increasing warnings. Certainly there are some bugfixes where it makes sense; typically this is when a slightly higher subset of the same problematic pattern starts being warned.

In this case, all imports are warned or not warned the same; exports are suddenly warned about. It's an entirely different coding pattern that's being warned on, suddenly, and with no ability to disable it.

[platinumazure]

I think I'd like to see this behind an option for now. If we want to flip the default for the option in a major release, I'd be okay with doing so at that time.

@ilyavolodin @not-an-aardvark @ljharb @j-f1 Thoughts?

[ljharb]

Sounds great! I only want to ensure i can independently control this new behavior; what the default is doesn’t concern me.

[ilyavolodin]

I'm fine with putting it behind an option.

[not-an-aardvark]

I'm not going to block adding an option, but I think this should be the default behavior at some point, and it seems unfortunate that we would have to continue maintaining the option forever for a behavior that doesn't make sense.

[ljharb]

@not-an-aardvark vN: add option. vN+1: switch option default, deprecate option. vN+2: remove option.

Majors are cheap, because we have lots of integers :-) it's not necessarily forever.

[not-an-aardvark]

Actually, we've committed to never making backwards-incompatible schema changes, even in major versions (see here).

[ljharb]

Fair (although that linked document only talks about not removing rules; not about not removing options)

[j-f1]

@not-an-aardvark @platinumazure In light of the discussion above, should I put this change behind an option?

[platinumazure]

TSC Summary:

Issue #9678 was originally filed as a bug report, asserting that no-restricted-imports does not handle an import that is re-exported. The issue was accepted as a bug. However, with the pull request that was submitted (this PR), we've had one user (who maintains a popular guide) request that it be put behind an option for backward compatibility.

TSC Question:

Should this be accepted as a bug with default behavior change (as semver-minor per our policy), knowing that it will cause some pain? Should this be accepted as a behavior change behind a new option (also semver-minor, but with no pain)? Or should we accept this as a bug with default behavior change, but choose not to merge in a semver-minor release and instead treat this as a full breaking change (semver-major)?

[platinumazure]

@j-f1 As you've no doubt seen, I've decided to have the TSC review it in our next meeting. That meeting is scheduled for tomorrow, so hopefully we can get a decision soon. Thanks for your patience!

[not-an-aardvark]

In the TSC meeting a few days ago, the TSC decided to add an option for this behavior, but make the default behavior to warn on this case.

[j-f1]

Sorry for the delay — I’ve been pretty busy and haven’t had a chance to take a good look at this.

Given this rule’s schema:

https://github.com/j-f1/forked-eslint/blob/a8745f74e56ac423152814f8413ac0eef8481ac3/lib/rules/no-restricted-imports.js#L64-L76

How could I add this option in a backward-compatible way?

[not-an-aardvark]

Would it work to only add the option when the user supplies an object? So the new schema would be something like:

        schema: {
            anyOf: [
                arrayOfStringsOrObjects,
                {
                    type: "array",
                    items: {
                        type: "object",
                        properties: {
                            paths: arrayOfStringsOrObjects,
                            patterns: arrayOfStrings,
                            allowReexports: boolean
                        },
                        additionalProperties: false
                    },
                    additionalItems: false
                }
            ]
        }

...and when the string option is provided, the rule would run as if allowReexports is false.

[j-f1]

I haven’t been able to do this recently (I guess my priorities have been elsewhere :/), so I’m going to close this out, but if someone wants to finish this up, please feel free to start from this branch!