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

Fix ignoreFiles being evaluated case-sensitively on Windows #5594

Open
adalinesimonian opened this issue Oct 12, 2021 · 9 comments
Open

Fix ignoreFiles being evaluated case-sensitively on Windows #5594

adalinesimonian opened this issue Oct 12, 2021 · 9 comments
Labels
status: ask to implement ask before implementing as may no longer be relevant

Comments

@adalinesimonian
Copy link
Member

adalinesimonian commented Oct 12, 2021

Tested on Stylelint 14

ignoreFiles is evaluated case-sensitively on Windows, leading to a few issues downstream in vscode-stylelint. I stumbled into this while migrating from the deprecated vscode-languageserver URI API to the new vscode-uri API. The new API generates lowercase drive letters for paths instead of uppercase letters as the old API did.

Let's say the project is in C:\Users\Adaline\Code\my-project. Suppose that there is a file named should-be-ignored.css and the Stylelint config is:

{ "ignoreFiles": "*-ignored.css" }

If you pass C:\Users\Adaline\Code\my-project\should-be-ignored.css to Stylelint, it ignores the file, as it should. But if you pass c:\...\should-be-ignored.css or any other case of any part of the path, Stylelint doesn't ignore it even though it can read the file correctly using the differently-cased path.

Node.js's API's behaviour, for reference, correctly sees these paths as equal on Windows:

const path = require('path');

path.win32.relative('C:\\Path\\to\\file.css', 'c:\\path\\To\\File.css')
// Evaluates to ''

I haven't checked any other config properties or functionality, so I'm unaware if there are other cases of this behaviour elsewhere in the project.

@jeddy3
Copy link
Member

jeddy3 commented Oct 13, 2021

Thanks for the report. Labelling as ready to implement.

@jeddy3 jeddy3 added the status: ready to implement is ready to be worked on by someone label Oct 13, 2021
@psychobolt
Copy link

psychobolt commented Feb 23, 2022

stylelint is using process.cwd() , which can potentially return lower-case in environments.
nodejs/node#6624

I think we should avoid using process.cwd() direct values or indexing keys. It creates a lot of potential issues with caching. Here we can see it can store the same config twice with using upper-case drive path in codeFilename and when respectively, no upper-case drive config path is defined in options...

Case:

./.stylelintrc.json

{
  "processors": ["stylelint-processor-styled-components"],
  "extends": ["stylelint-config-standard-scss", "stylelint-config-styled-components"],
  "rules": {
    "selector-type-case": ["lower", { "ignoreTypes": ["/^\\$\\w+/"] }],
    "selector-type-no-unknown": [true, { "ignoreTypes": ["/-styled-mixin/", "/^\\$\\w+/"] }],
    "value-keyword-case": ["lower", { "ignoreKeywords": ["dummyValue"] }],
    "declaration-colon-newline-after": null
  },
  "overrides": [{
    "files": ["./**/*.js"],
    "customSyntax": "postcss-scss"
  }]
}

./foo.js

const foo = styled.div`
  .bar {
    line-height: 1em // CssSyntaxError missing semi-colon
  }
`;

./stylelint-runner-exception.js

import { logResult, logError } from './lintLogger';

await stylelint.lint({ codeFilename: 'C:\\Path\\to\\project\foo.js'  }).then(logResult).catch(logError);  
// exception from stylelint-processor-styled-component in logs e.g. Cannot access property "3" of undefined... instead of reporting missing semi-colon

// debugging details:
// calls getConfigForFile(stylelint._options.configFile); // searchPath defaulted to process.cwd()
// calls augmentConfigFull(config); // See https://github.com/stylelint/stylelint/blob/29acc54ef08174a6b0caa69d093b9e3bd3ba897f/lib/augmentConfig.js#L82
// - calls addProcessorFunctions, processorCache misses.
// - Require and add processor to cache from config

// linting begins with lintSource
// will fetch config again to check for ignore files. See line https://github.com/stylelint/stylelint/blob/a7d6e5651a7e43a7978c18798fa88e4fc2473c8f/lib/lintSource.js#L51
// calls getConfigForFile((stylelint._options.configFile || inputFilePath); // configFile is undefined
// calls augmentConfigFull(config);
// - calls addProcessorFunctions, processorCache misses.
// - Require and add processor to cache from config



console.log(Array.from(stylelint._specifiedConfigCache.keys())) // has both upper and lower case caches. Each config has their own caches, processors, plugins, etc! 
/*
* [
*  'c:\\Path\\to\\project\\.stylelintrc.json',
*  'C:\\Path\\to\\project\\.stylelintrc.json'
* ]
*/

console.log(stylelint._specifiedConfigCache.get('C:\\Path\\to\\project\.stylelintrc.json') === stylelint._specifiedConfigCache.get('c:\\Path\\to\\project\.stylelintrc.json')); 
// false

./stylelint-runner-no-exeception1.js

import { logResult, logError } from './lintLogger';

await stylelint.lint({ configFile: 'C:\\Path\\to\\project\.stylelintrc.json', codeFilename: 'C:\\Path\\to\\project\foo.js'  }).then(logResult).catch(logError);;
// logs the semi-colon error. Works!

console.log(Array.from(stylelint._specifiedConfigCache.keys()))
/*
* [
*  'C:\\Path\\to\\project\\.stylelintrc.json'
* ]
*/

./stylelint-runner-no-exception2.js

import { logResult, logError } from './lintLogger';

await stylelint.lint({ codeFilename: 'c:\\Path\\to\\project\foo.js'  }).then(logResult).catch(logError);
// also works

console.log(Array.from(stylelint._specifiedConfigCache.keys()))
/*
* [
*  'c:\\Path\\to\\project\\.stylelintrc.json'
* ]
*/

Copy link
Contributor

This issue is older than one month. Please ask before opening a pull request, as it may no longer be relevant.

@github-actions github-actions bot added status: ask to implement ask before implementing as may no longer be relevant and removed status: ready to implement is ready to be worked on by someone labels Jan 22, 2024
@Mouvedia
Copy link
Member

What about using an util to normalize the paths?

e.g.

return path
    .resolve(folder)
    .split(path.sep)
    .join("/")
    .replace(/^([a-z]):/, (_, driveLetter) => driveLetter.toLowerCase() + ":");

@ybiquitous
Copy link
Member

Can we use normalize-path? We have already it as a dependency:

"normalize-path": "^3.0.0",

@Mouvedia
Copy link
Member

Mouvedia commented Jul 12, 2024

Can we use normalize-path? We have already it as a dependency:

"normalize-path": "^3.0.0",

Nop it doesn't lowercase the drive letter.
cf https://npm.runkit.com/normalize-path

related: nodejs/node-v0.x-archive@016e084

@ybiquitous
Copy link
Member

Can we reproduce this bug in our CI? If yes, it seems that we can write a patch using toLowerCase().

@Mouvedia
Copy link
Member

Mouvedia commented Jul 16, 2024

        // TODO: Remove once fixed upstream
        test('should upper-case drive letters on Windows (Stylelint bug #5594)', async () => {

ref: https://github.com/stylelint/vscode-stylelint/blob/d37eb22458751a4b983c44f871290ee90ba0e53f/src/utils/stylelint/__tests__/stylelint-runner.ts#L66-L67

@ybiquitous
Copy link
Member

Nice. It seems possible to bring the test case from vscode-stylelint.

@jeddy3 jeddy3 removed the type: bug label Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ask to implement ask before implementing as may no longer be relevant
Development

No branches or pull requests

5 participants