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

Support for "message" with "patterns" in no-restricted-imports #11843

Closed
mikeplis opened this issue Jun 15, 2019 · 29 comments · Fixed by #14580
Closed

Support for "message" with "patterns" in no-restricted-imports #11843

mikeplis opened this issue Jun 15, 2019 · 29 comments · Fixed by #14580
Assignees
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

@mikeplis
Copy link

mikeplis commented Jun 15, 2019

Creating a new request for a closed issue, as suggested here.

What rule do you want to change? no-restricted-imports

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

How will the change be implemented? (New option, new default behavior, etc.)? Allow a new format for an existing option

Please provide some example code that this change will affect:

// I can currently customize the error message for restricted path imports like this
import * as moment from 'moment';
// But I can't customize the error message for restricted pattern imports like this
import { Button } from '../../framework/buttons';

What I'd like to do in my ESLint config:

{
  "rules": {
    "no-restricted-imports": ["error", {
      "paths": [{"name": "moment", "message": "Don't import moment"}],
      "patterns": [{"name": "**/framework/*", "message": "Only import from the root of /framework"}]
    }]
  }
}

What does the rule currently do for this code? I can't customize the error message for the restricted pattern import

What will the rule do after it's changed? It should allow me to customize the error message for the restricted pattern import

Are you willing to submit a pull request to implement this change? Sure, though I'd like to get some clarity on how difficult the "large option schema change" mentioned here would be.

@mikeplis mikeplis 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 Jun 15, 2019
@g-plane g-plane added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Jun 17, 2019
@eslint-deprecated eslint-deprecated bot added the auto closed The bot closed this issue label Jul 18, 2019
@eslint-deprecated
Copy link

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.

@platinumazure
Copy link
Member

Okay, we've had two team members review this and be in favor of the change but I think more folks can look at this.

I'll champion.

@eslint/eslint-team Can we review this please?

@platinumazure platinumazure reopened this Jul 18, 2019
@platinumazure platinumazure self-assigned this Jul 18, 2019
@platinumazure platinumazure removed the auto closed The bot closed this issue label Jul 18, 2019
@kaicataldo
Copy link
Member

@platinumazure Are you still championing this?

@martin-cech
Copy link

+1 for this to express interest :-)

@husseinbob
Copy link

Been smashing my head against a wall trying to find a way to do this. +1 interest

@nzakas
Copy link
Member

nzakas commented Feb 14, 2020

Folks, no need to leave a comment to express interest. There’s definitely enough interest. What we need now is someone to figure out if it’s possible to create a rule schema to validate such options while maintaining backwards compatibility.

@platinumazure platinumazure removed their assignment Feb 14, 2020
@platinumazure
Copy link
Member

As I'm no longer a team member, I have removed my assignment and will not/cannot champion this rule. Would anyone else on the team like to champion this and drive this to completion?

@mdjermanovic
Copy link
Member

What I'd like to do in my ESLint config:

{
  "rules": {
    "no-restricted-imports": ["error", {
      "paths": [{"name": "moment", "message": "Don't import moment"}],
      "patterns": [{"name": "**/framework/*", "message": "Only import from the root of /framework"}]
    }]
  }
}

An issue with this model might be that patterns can be related, as noted in this comment by @platinumazure.

The actual implementation treats "patterns" array as lines in .gitignore:

/* eslint no-restricted-imports:["error", {
    patterns:["foo/*", "!foo/good"]
}]*/

import bad from "foo/bad"; // error
import good from "foo/good"; // ok

Technically, "foo/good" is allowed because ["foo/*", "!foo/good"] as a whole doesn't match "foo/good" (positions in array also matter).

Schema proposed in #9846 covers this.

An alternative with the path/patterns object could be something like:

{
    "rules": {
        "no-restricted-imports": ["error", {
            "paths": [{
                "name": "moment", 
                "message": "Don't import moment"
            }],
            "patterns": [{
                "group": ["import1/private/*"], 
                "message": "Do not use any modules in import1/private"
            }, {
                "group": ["import2/*", "!import2/good"], 
                "message": "Do not use any modules in import2 except import2/good"
            }]
        }]
    }
}

"patterns" can be either:

  • Flat list of strings. It would work the same as it works now.
  • List of { group, message } objects.
    • group is a mandatory array of strings with at least 1 element.
    • message is optional.

Each group is a test for itself. If it matches, the import is restricted.

To avoid any confusion about the behavior of string elements in a configuration with groups (flat list of strings basically represents a group), I think that a mix of strings and objects in the "patterns" array should not be allowed: all patterns must be in group arrays.

@eslint-deprecated eslint-deprecated bot added the auto closed The bot closed this issue label Mar 17, 2020
@eslint-deprecated
Copy link

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.

@mdjermanovic mdjermanovic removed the auto closed The bot closed this issue label Mar 17, 2020
@mdjermanovic mdjermanovic self-assigned this Mar 17, 2020
@mdjermanovic
Copy link
Member

I'll champion this.

@mdjermanovic mdjermanovic reopened this Mar 17, 2020
@osdiab
Copy link

osdiab commented Mar 21, 2020

I am also interested in this happening - how can I help?

@mysticatea
Copy link
Member

I'm wondering if we can simplify the options of the rule, similar to node/no-restricted-import.

@mdjermanovic mdjermanovic removed the evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion label Mar 28, 2020
@nzakas
Copy link
Member

nzakas commented Jan 13, 2021

@mdjermanovic @mysticatea how should we proceed here?

@GiancarlosIO
Copy link

+1 🙏

@nzakas nzakas added this to Needs Triage in Triage via automation Mar 10, 2021
@nzakas nzakas moved this from Needs Triage to Evaluating in Triage Mar 10, 2021
@mdjermanovic
Copy link
Member

If we are writing a new rule, then I think we should definitely choose one of the schemas proposed in this comment by @mysticatea.

But, since we already have the rule with defined schema, I'd vote for this solution, as it looks like a minimal addition to the existing schema and the logic that handles configuration.

@nzakas
Copy link
Member

nzakas commented Mar 18, 2021

That sounds reasonable. Is the next step to produce an RFC? (This issue is flagged as needing a design, so double checking.)

@mdjermanovic
Copy link
Member

That sounds reasonable. Is the next step to produce an RFC? (This issue is flagged as needing a design, so double checking.)

If we all agree on this schema, then I think an RFC wouldn't be necessary, as it's a small backwards-compatible addition.

If that schema doesn't look good, and/or we'd prefer to redesign the rule instead, then it would be probably better to go through the RFC process.

@nzakas
Copy link
Member

nzakas commented Mar 20, 2021

Works for me. If no one else objects, let’s move forward with that.

@nzakas nzakas moved this from Evaluating to Ready to Implement in Triage Mar 23, 2021
@nzakas nzakas removed the needs design Important details about this change need to be discussed label Mar 23, 2021
@mifrej

This comment has been minimized.

@ljharb

This comment has been minimized.

@mifrej

This comment has been minimized.

@GiancarlosIO
Copy link

😢

@nzakas
Copy link
Member

nzakas commented Apr 29, 2021

Folks, let’s keep the discussion professional, please. Sarcasm isn’t helpful.

In case it wasn’t clear, this issue has been accepted and is available for anyone to implement. This schema is the agreed-upon approach. We are looking for a community member to implement this as we are busy preparing ESLint for v8.0.0.

@lexholden
Copy link
Contributor

I needed this for a big project, so made it as a custom rule locally as per to the defined schema and opened the above PR for feedback. #14580

Happy to put a bit of time into getting it mergeable if there's any change suggestions.

lexholden added a commit to lexholden/eslint that referenced this issue May 8, 2021
@mdjermanovic mdjermanovic moved this from Ready to Implement to Pull Request Opened in Triage May 20, 2021
Triage automation moved this from Pull Request Opened to Complete May 21, 2021
mdjermanovic added a commit that referenced this issue May 21, 2021
…) (#14580)

* Update: no-restricted-imports rule with custom messages (fixes #11843)

* Update docs/rules/no-restricted-imports.md

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

* Update docs/rules/no-restricted-imports.md

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

* Code review feedback - better test cases and schema change

* Doc updates

* Added correct/incorrect examples to docs

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Nov 18, 2021
@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 Nov 18, 2021
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.