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

Rule Change: Add importNames to patterns in no-restricted-imports rule #15236

Closed
1 task done
lekterable opened this issue Oct 31, 2021 · 5 comments
Closed
1 task done
Labels
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

@lekterable
Copy link

What rule do you want to change?

no-restricted-imports

What change to do you want to make?

Generate more warnings

How do you think the change should be implemented?

Other

Example code

module.exports = {
  rules: {
    'no-restricted-imports': [
      'error',
      {
        patterns: [
          {
            group: ['emotion-theming', '@react-navigation/native'],
            importNames: ['useTheme'], // The Change
            message: "Use 'useTheme' from the 'theme' module instead."
          }
        ]
      }
    ]
  }
}

What does the rule currently do for this code?

It's a change in the rule's options in eslint config. What I'm proposing is to add an importNames property for patterns same as we have in paths.

Sometimes you want to restrict the same import from multiple sources, you can accomplish that by providing separate objects for each source, but this way is redundant as you need to provide a name and a message each time.

E.g.

Instead of:

paths: [
  {
    name: 'emotion-theming',
    importNames: ['useTheme'],
    message: "Use 'useTheme' from the 'theme' module instead.",
  },
  {
    name: '@react-navigation/native',
    importNames: ['useTheme'],
    message: "Use 'useTheme' from the 'theme' module instead.",
  },
...
],

We could have:

patterns: [
  {
    group: ['emotion-theming', '@react-navigation/native', ...],
    importNames: ['useTheme'],
    message: "Use 'useTheme' from the 'theme' module instead.",
  },
],

What will the rule do after it's changed?

The rule would work exactly the same, it's just an easier way of providing multiple sources of the same restricted import.

Participation

  • I am willing to submit a pull request to implement this change.

Additional comments

No response

@lekterable lekterable added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules labels Oct 31, 2021
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Oct 31, 2021
@nzakas
Copy link
Member

nzakas commented Nov 2, 2021

Thanks for the suggestion. You didn’t actually explain the proposal (code examples aren’t enough), how it would work, or how this would benefit users. Please take some time to document those on this issue and then we can take a look.

@nzakas nzakas moved this from Needs Triage to Triaging in Triage Nov 2, 2021
@lekterable
Copy link
Author

Hi @nzakas , I thought the example I provided would be enough, can you tell me which part is not clear?

My idea is to add the importNames property to patterns just as we have in paths and it would work same way, which is it would allow you to restrict only some parts of the module instead of the whole import.

Let's say we want to restrict an import of useTheme because we have our own custom hook and we want to do this from multiple sources, in my case Intellij suggests emotion-theming and @react-navigation/native which I would like to restrict because I don't want them to be used in my project.

Currently to do that I would have to go with paths like this:

paths: [
  {
    name: 'emotion-theming',
    importNames: ['useTheme'],
    message: "Use 'useTheme' from the 'theme' module instead.",
  },
  {
    name: '@react-navigation/native',
    importNames: ['useTheme'],
    message: "Use 'useTheme' from the 'theme' module instead.",
  },
...
],

But I don't really like that as it forces me to provide a name and a message each time even though they are the same.

If patterns would also have the importNames property I could just go with:

patterns: [
  {
    group: ['emotion-theming', '@react-navigation/native', ...],
    importNames: ['useTheme'],
    message: "Use 'useTheme' from the 'theme' module instead.",
  },
],

and quickly add more sources if they come up without any boilerplate.

Is it clearer now?

@nzakas
Copy link
Member

nzakas commented Nov 4, 2021

Yes, much clearer, thank you.

This seems like a reasonable addition to me, and may have been an oversight based on how paths already supports it.

Let’s see what the team thinks.

@nzakas nzakas moved this from Triaging to Feedback Needed in Triage Nov 4, 2021
@mdjermanovic
Copy link
Member

This looks like a duplicate of #14274, as the proposed schema is same as in #14274 (comment).

paths: [
  {
    name: 'emotion-theming',
    importNames: ['useTheme'],
    message: "Use 'useTheme' from the 'theme' module instead.",
  },
  {
    name: '@react-navigation/native',
    importNames: ['useTheme'],
    message: "Use 'useTheme' from the 'theme' module instead.",
  },
...
],
patterns: [
  {
    group: ['emotion-theming', '@react-navigation/native', ...],
    importNames: ['useTheme'],
    message: "Use 'useTheme' from the 'theme' module instead.",
  },
],

Note that this wouldn't be equivalent. name: 'emotion-theming' restricts only import ... from 'emotion-theming', but 'emotion-theming' in patterns is a gitignore pattern, so per the gitignore rules it may match at any level, and also restricts paths underneath the matched part:

/* eslint no-restricted-imports: ["error", {

  patterns: [
    {
      group: ['emotion-theming', '@react-navigation/native'], 
      message: "Use 'useTheme' from the 'theme' module instead.",
    },
  ]
}]*/

import { useTheme } from "emotion-theming"; // error

import { useTheme as useTheme1 } from "foo/emotion-theming"; // also error

import { useTheme as useTheme2 } from "emotion-theming/foo"; // also error

online demo

@lekterable
Copy link
Author

yup, seems like they are the same, thanks for explanation and for pointing me there!
closing

Triage automation moved this from Feedback Needed to Complete Nov 4, 2021
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators May 4, 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 May 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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

No branches or pull requests

3 participants