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

envs in overrides was not applied #11577

Closed
aladdin-add opened this issue Apr 2, 2019 · 14 comments · Fixed by #11799 · May be fixed by Omrisnyk/npm-lockfiles#130
Closed

envs in overrides was not applied #11577

aladdin-add opened this issue Apr 2, 2019 · 14 comments · Fixed by #11799 · May be fixed by Omrisnyk/npm-lockfiles#130
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 bug ESLint is working incorrectly core Relates to ESLint's core APIs and features

Comments

@aladdin-add
Copy link
Member

Tell us about your environment

  • ESLint Version: v5.16.0
  • Node Version: v10.15.3
  • npm Version: 6.9.0

What parser (default, Babel-ESLint, etc.) are you using?

Please show your full configuration:

Configuration
  "eslintConfig": {
    "extends": [
      "eslint:recommended"
    ],
    "overrides": [
      {
        "files": [
          "./lib/*.js"
        ],
        "env": {
          "browser": true
        }
      }
    ]
  },

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

'use strict';

window.console.log('hello');

What did you expect to happen?
not error reported, since it has setting env "browser": true

What actually happened? Please include the actual, raw output from ESLint.

/Users/weiran/repo/work/eslint-issue/lib/index.js
  3:1  error  'window' is not defined  no-unde

Are you willing to submit a pull request to fix this bug?
yes

@aladdin-add aladdin-add added bug ESLint is working incorrectly core Relates to ESLint's core APIs and features accepted There is consensus among the team that this change meets the criteria for inclusion labels Apr 2, 2019
@aladdin-add aladdin-add self-assigned this Apr 2, 2019
@aladdin-add
Copy link
Member Author

I've created a repro: https://github.com/aladdin-add/eslint-issue-11577

@mysticatea
Copy link
Member

How about removing ./?

  "eslintConfig": {
    "extends": [
      "eslint:recommended"
    ],
    "overrides": [
      {
        "files": [
          "lib/*.js"
        ],
        "env": {
          "browser": true
        }
      }
    ]
  },

@aladdin-add
Copy link
Member Author

it works! I'm not sure if it is a bug.

@aladdin-add aladdin-add added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed accepted There is consensus among the team that this change meets the criteria for inclusion labels Apr 2, 2019
@mysticatea
Copy link
Member

Indeed, I agree that it's surprising if ./lib/*.js and lib/*.js have different behaviors.

minimatch looks handling the two as different patterns:
image

It may be isaacs/minimatch#30.
Probably we can remove ./ and .\ at the start of paths then pass it to minimatch.

@kaicataldo
Copy link
Member

I would argue that this is unexpected/unintended behavior and is a bug.

@mysticatea mysticatea added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels May 29, 2019
@g-plane
Copy link
Member

g-plane commented May 30, 2019

How about switching to https://github.com/micromatch/micromatch which is also widely used?

@mysticatea
Copy link
Member

I'm not positive to switch library because I'm not sure how the switch changes ESLint's behavior.

@g-plane
Copy link
Member

g-plane commented Jun 3, 2019

They documented a notable difference: https://github.com/micromatch/micromatch#backslashes , and can we mark it as a breaking change if we decide to switch?

@mysticatea
Copy link
Member

Nice document. Hmm, I think that ESLint pretty depends on the Minimatch's spec that replaces \ by / on Windows.

@jonschlinkert
Copy link

Minimatch's spec that replaces \ by / on Windows.

TLDR; fwiw the reason that micromatch doesn't replace \ is that it's unsafe. Occasionally users will create issues about it, but it's easy to resolve and results in educating a user. Zero issues would be best, but we'd rather have issues about implementation problems than vulnerabilities and ReDoS.


If you're interested in a little more info about why it's unsafe to convert backslashes...

Since globs are converted to regular expressions, when backslashes are converted to forward slashes it can change the intent of the glob and match things that shouldn't be matched, or create a malicious regular expression that can lead to DoS. I've seen many naive and clever attempts at guessing when backslashes should be converted, but there are so many different edge cases that it's just not worth the risk and headache - especially since globs are often joined to path parts before passing them to a matcher, which further complicates the guesswork of determining whether or not 1) slashes can be safely converted, and that 2) converting slashes will not change the intent of the glob pattern (and ultimately the regex).

Also fwiw, in case you're concerned about breaking people's configs and getting a bunch of issues. Micromatch has something like 2.5m dependents, and believe me, we were cringing when we made the decision to enforce this with users. But I have to say it wasn't nearly as bad as we anticipated. We'll get an issue about it every now and then... maybe 4-6 a year. But I don't recall seeing any issues about this in months, and we even have a tool that "watches" issues on every repository on GitHub so that we can be notified in near real-time when something needs to be triaged.

(Please don't take my comment as an attempt to persuade you to use Micromatch. I don't expect that and wouldn't judge you for not switching. I just figured I'd drop by and share some things we've learned.)

@mysticatea
Copy link
Member

After some investigating, I don't think the switching by this issue is reasonable because Micromatch has the same behavior about ./ as Minimatch. The switch is not related to this issue.

image

@g-plane
Copy link
Member

g-plane commented Jun 3, 2019

Well, just ignore my suggestion.

@jonschlinkert
Copy link

because Micromatch has the same behavior about ./ as Minimatch.

Nope. Two things:

  1. Your config is wrong in the second and third examples. You're passing a glob with a slash in it AND specifying matchBase in the options, which tells micromatch to only match against the basename of the file path.
  2. Micromatch gives you the ability to strip the leading ./ before matching. Just pass a format function on the options, as in the below example.
// strip leading "./" and normalize windows slashes to posix slashes
const format = str => str.replace(/^\.\/, '').replace(/\\/, '/');

@mysticatea
Copy link
Member

Your config is wrong in the second and third examples. You're passing a glob with a slash in it AND specifying matchBase in the options, which tells micromatch to only match against the basename of the file path.

This issue is that ./lib/file.js and lib/file.js has different behavior. It's the same on both packages.

Micromatch gives you the ability to strip the leading ./ before matching. Just pass a format function on the options, as in the below example.

I'm not sure if the option is useful. We can fix paths before we pass it to minimatch/micromatch.

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Dec 3, 2019
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Dec 3, 2019
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 bug ESLint is working incorrectly core Relates to ESLint's core APIs and features
Projects
None yet
5 participants