Breaking: Update ignore to v5

[bmish]

Prerequisites checklist

• I have read the contributing guidelines.

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

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

What changes did you make? (Give an overview)

Update dependency.

https://github.com/kaelzhang/node-ignore/releases/tag/5.0.0

Fixes #13294.

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

No.

[aladdin-add]

As a breaking change, it seems an enjoyable time to upgrade it as eslint v8.0.0.

[mdjermanovic]

Can you provide an overview of breaking changes for ESLint users? This wasn't announced for v8.0.0, so it seems like the change would have to wait until v9.0.0.

[bmish]

@mdjermanovic It looks like the potential breaking change is that ignore v5 will throw an exception if an invalid path is passed to our no-restricted-imports or no-restricted-modules rules. If we can't include this breaking change in this release, then I could investigate catching the exception in our two rules so that the upgrade would not be a breaking change (as discussed in #13294). If there isn't time for that, we could target it for v9 instead.

[nzakas]

If the change just affects a couple of rules, I think we can include this in v8.0.0. We don't announce all changes to all rules ahead of time, and we can add this into the release notes.

[bmish]

I'm moving this PR back to draft since I've identified some missing test cases that it breaks for the no-restricted-imports / no-restricted-modules rules:

• import relativeWithPatterns from '../foo'; when using no-restricted-imports rule with the patterns options
• import absoluteWithPatterns from '/foo'; when using no-restricted-imports rule with the patterns options
• var relativeWithPatterns = require('../foo'); when using no-restricted-modules rule with the patterns options
• var absoluteWithPatterns = require('/foo'); when using no-restricted-modules rule with the patterns options

I'm going to add the missing test cases to the rules in #14910 to ensure they aren't mistakenly broken going forward. After that, we can reevaluate if it's possible to pull off this upgrade.

[mdjermanovic]

Note that @eslint/eslintrc, which is still a dependency, also uses the ignore package, so we'll have two different version installed (ignore v4 for eslintrc, ignore v5 for ESLint).

[nzakas]

Should we upgrade eslintrc and/or not upgrade here?

[bmish]

Should we upgrade eslintrc and/or not upgrade here?

Ideally, we would upgrade in both places of course, but I haven't yet found a way to upgrade here without breaking important test cases for these two rules. If we don't find a safe upgrade path in the coming days, it should be okay to have a different version in each package.

[aladdin-add]

Given these cases are very edge (not well tested and documented), I was wondering how many users were really using the "feature". What do you think just removing the cases and document the limitation (in eslint v8.0.0)?

[bmish]

Given these cases are very edge (not well tested and documented), I was wondering how many users were really using the "feature". What do you think just removing the cases and document the limitation (in eslint v8.0.0)?

I don't think they're edge cases at all. I've used relative imports/requires extensively in many projects. ESLint itself has them everywhere as well.

For example, this line from inside ESLint causes no-restricted-modules to crash when the rule is enabled with ANY pattern option.

/* eslint no-restricted-modules: ["error", { pattern:"foo" }] */

const { SourceCode } = require("../../../lib/source-code");

So these are indeed important test cases that were missing.

[aladdin-add]

oh yes, you are right! I just misread the failing tests.

[aladdin-add]

a solution is to skip checking abs/rel path: aladdin-add@994b661.

its dark side is patterns cannot be ../foo and /foo. see the test result: https://github.com/aladdin-add/eslint/pull/4/checks?check_run_id=3301845971#step:5:52

[bmish]

a solution is to skip checking abs/rel path: aladdin-add@994b661.

its dark side is patterns cannot be ../foo and /foo. see the test result: aladdin-add/eslint#4 (checks)

I don't think it's acceptable to skip checking absolute / relative import paths. That unnecessarily cripples the rule for what is likely to be a common use case for many people, for little to no tangible benefit.

Instead, I just pushed a WIP workaround where we simply strip the / or ../ prefix from the import path before checking it. So far, it looks like we can safely ignore such prefixes, as long as the patterns provided to the rule don't contain them as well (we can throw an exception if they do). Still need to investigate this approach a bit further to determine if it's completely suitable.

[ljharb]

Without the leading dot it means something very different tho - if i have one import from ./foo/bar and another from foo/bar, those could be configured to be treated differently, since one is relative and one bare, no?

[bmish]

Without the leading dot it means something very different tho - if i have one import from ./foo/bar and another from foo/bar, those could be configured to be treated differently, since one is relative and one bare, no?

Correct, my workaround doesn't allow people to configure the rule to treat them differently. So overall I'm not happy with the options we have for using ignore v5 and would rather not upgrade than reduce the rule's functionality. The v5 version of ignore is no longer perfectly suited to our use case. CC: @kaelzhang

[nzakas]

Just a note: if the upgrade isn’t straight forward, we don’t need to upgrade. The amount of effort should be in line with the value we are getting. If the effort is too great and the value is low, let’s just skip the upgrade. (This is what happened with the Ajv upgrade — too many issues and we were fine with the current version, so we opted not to upgrade.)