Skip to content
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

no-restricted-imports should support importNames on patterns exclusions #14274

Closed
xdumaine opened this issue Mar 30, 2021 · 18 comments · Fixed by #16059
Closed

no-restricted-imports should support importNames on patterns exclusions #14274

xdumaine opened this issue Mar 30, 2021 · 18 comments · Fixed by #16059
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects

Comments

@xdumaine
Copy link
Contributor

What rule do you want to change?

no-restricted-imports

Does this change cause the rule to produce more or fewer warnings?

Neither, it makes configuration more powerful

How will the change be implemented? (New option, new default behavior, etc.)?

New options to the rule schema

Please provide some example code that this change will affect:

import { Foo, Bar } from '../../my/relative-module'
import { Foo } from '../../../my/relative-module'

What does the rule currently do for this code?

It's not currently possible to ban importing only a named import from a pattern ban. Nor is it possible to include a message for those bans.

What will the rule do after it's changed?

Could be configured as follows:

'no-restricted-imports: [
  'error',
  {
    patterns: [ '**/my/relative-module'],
    importNames: ['Foo'], // currently not supported for patterns
    message: 'Don't import Foo from my/relative-module. Instead import from @repo/foo', // currently not supported for patterns
  }
]

Are you willing to submit a pull request to implement this change?

Potentially, but I'd like some guidance and my time is limited due to having young kids.

@xdumaine xdumaine added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules triage An ESLint team member will look at this issue soon labels Mar 30, 2021
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Mar 30, 2021
@mdjermanovic mdjermanovic moved this from Needs Triage to Triaging in Triage Mar 31, 2021
@mdjermanovic mdjermanovic removed the triage An ESLint team member will look at this issue soon label Mar 31, 2021
@mdjermanovic
Copy link
Member

Hi @xdumaine, thanks for the issue!

It's not currently possible to ban importing only a named import from a pattern ban.

Is there a use case for restricting only a certain named import from a group of files that match the pattern?

Nor is it possible to include a message for those bans.

This is already accepted in #11843

@xdumaine
Copy link
Contributor Author

The use case we have is that graphql codegen generates types for our resolvers and also exports some type helpers like Maybe, etc. Devs often use those but inadvertently cause circular dependencies.

I'd like to say "don't use the Maybe export from the generated code files" without banning other exports from those same files.

@mdjermanovic
Copy link
Member

Thanks for the explanation, makes sense to me. I support this proposal 👍, let's hear what other team members think about it.

Given the accepted change #11843 (comment), configuration would be:

"no-restricted-imports": ["error", {            
    "patterns": [{
        "group": ["**/my/relative-module"],
        "importNames": ["Foo"],
        "message": "Don't import Foo from my/relative-module. Instead import from @repo/foo"
    }]
}]

@github-actions
Copy link

Oops! It looks like we lost track of this issue. @eslint/eslint-tsc what do we want to do here? This issue will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Oct 15, 2021
@ljharb
Copy link
Sponsor Contributor

ljharb commented Oct 15, 2021

It'd be great if someone had time to make a PR (assuming it's accepted?)

@snitin315
Copy link
Contributor

"no-restricted-imports": ["error", {            
    "patterns": [{
        "group": ["**/my/relative-module"],
        "importNames": ["Foo"],
        "message": "Don't import Foo from my/relative-module. Instead import from @repo/foo"
    }]
}]

So, the above config will result in the following behaviour:

import { Foo } from '../../../my/relative-module' // error
import { Foo, Bar } from '../../../my/relative-module' // error
import { Bar } from '../../../my/relative-module' // ok

It makes sense to me as well. I am also 👍🏻 on this. I can submit a PR for this if accepted.

@lekterable
Copy link

Coming here from #15236

Can this already be worked on or does it need some more approvals?
Also, @snitin315 are you still up to take this? if you're busy I might try 🙂

@github-actions
Copy link

github-actions bot commented Jan 7, 2022

Oops! It looks like we lost track of this issue. What do we want to do here? This issue will auto-close in 7 days without an update.

@Akridian
Copy link

So @snitin315, @lekterable, can someone try to made a PR. Suddenly have no experience in this field to make one.

@github-actions github-actions bot removed the Stale label Jan 14, 2022
@github-actions
Copy link

Oops! It looks like we lost track of this issue. What do we want to do here? This issue will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Mar 16, 2022
@ljharb
Copy link
Sponsor Contributor

ljharb commented Mar 16, 2022

bump

@github-actions github-actions bot removed the Stale label Mar 17, 2022
@jtwee
Copy link

jtwee commented Apr 26, 2022

I just came across this limitation when trying to disallow only a specific subset of exports from a given module. It works nicely for absolute paths, but to support relative paths I would need patterns. Is this likely to receive approval? And if so, would a PR attempt be welcome?

@xdumaine
Copy link
Contributor Author

@jtwee yep, from previous comments, looks like a PR would definitely be very helpful.

@github-actions
Copy link

Oops! It looks like we lost track of this issue. What do we want to do here? This issue will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Jun 25, 2022
@ljharb
Copy link
Sponsor Contributor

ljharb commented Jun 26, 2022

bump

@brandongregoryscott
Copy link
Contributor

I took a stab at this over the weekend - will throw up a PR for feedback later today!

@nzakas
Copy link
Member

nzakas commented Jun 28, 2022

@mdjermanovic if you think it’s a good idea then I’m 👍

@mdjermanovic
Copy link
Member

This feature seems to have enough support from the team, so I'm marking this as accepted, and we already have a PR: #16059

@mdjermanovic mdjermanovic moved this from Feedback Needed to Pull Request Opened in Triage Jun 28, 2022
@mdjermanovic mdjermanovic added the accepted There is consensus among the team that this change meets the criteria for inclusion label Jun 28, 2022
@mdjermanovic mdjermanovic changed the title no-restricted-imports should support importNames and message on patterns exclusions no-restricted-imports should support importNames on patterns exclusions Jun 28, 2022
Triage automation moved this from Pull Request Opened to Complete Jul 2, 2022
btmills pushed a commit that referenced this issue Jul 2, 2022
…16059)

* feat: add importNames support for restricted import patterns

Fixes #14274

* refactor: Report * import as error, add new messageIds, update docs

* docs: add code examples for patterns with importNames

* refactor: handle star import case separately w/ new messageIds

* refactor: add message assertions + additional test cases, remove early return to catch default import case

* refactor: require 1 unique importName in config, remove array coalesce
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Dec 30, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Dec 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
Archived in project
Triage
Complete
Development

Successfully merging a pull request may close this issue.

9 participants