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 overrides.files in config for glob patterns like *.vue #6525

Closed
sronveaux opened this issue Dec 14, 2022 · 12 comments · Fixed by #6547
Closed

Fix overrides.files in config for glob patterns like *.vue #6525

sronveaux opened this issue Dec 14, 2022 · 12 comments · Fixed by #6547
Assignees
Labels
status: wip is being worked on by someone type: bug a problem with a feature or rule

Comments

@sronveaux
Copy link

What steps are needed to reproduce the bug?

Well, I'll try to be clear here as it is not easy to explain...

I created a simple repo that you can clone to reproduce the bug: https://github.com/sronveaux/stylelint-bug-report

Here's the smallest code base I managed to get to and that can be used to reproduce the bug:

filesToLint/
|    |
|    ------> test.css
|    ------> test.html
|
stylelintInstall/
|    |
|    ------> node_modules/
|    ------> package.json
|
.stylelintrc

The .stylelintrc content is very simple:

{
  "extends": [
    "stylelint-config-standard"
   ],
  "overrides": [
    {
      "files": ["*.html", "**/*.html"],
      "customSyntax": "postcss-html"
    }
  ]
}

If I get to the stylelintInstall directory and run the following commands:

npx stylelint ../filesToLint/*.css
npx stylelint ../filesToLint/*.html

everything works as expected. However, if configBasedir is added:

npx stylelint --config-basedir . ../filesToLint/*.html

The .html files cannot be linted anymore and the following message is returned:

When linting something other than CSS, you should install an appropriate syntax, e.g. "postcss-html", and use the "customSyntax" option

../filesToLint/test.html
 1:1  ✖  Unknown word  CssSyntaxError

This can be made working back again by changing the content of the .stylelintrc file like this:

{
  "extends": [
    "stylelint-config-standard"
   ],
  "overrides": [
    {
      "files": ["../*.html", "../**/*.html"],
      "customSyntax": "postcss-html"
    }
  ]
}

However this manual patch doesn't work when used in conjuction with extensions like stylelint-config-standard-vue, not to mention how it fails when used through the VSCode extension...

What Stylelint configuration is needed to reproduce the bug?

{
  "extends": [
    "stylelint-config-standard"
   ],
  "overrides": [
    {
      "files": ["*.html", "**/*.html"],
      "customSyntax": "postcss-html"
    }
  ]
}

How did you run Stylelint?

CLI

Which version of Stylelint are you using?

14.16.0 under Windows 11

What did you expect to happen?

Shouldn't the glob patterns we write in the .stylelintrc file be relative to this config file instead of being interpreted differently when used in conjunction with --config-basedir ?

What actually happened?

It seems like the file matching in the overrides part of the config is dependent on the config-basedir as the problem can be solved by changing the glob pattern written in the overrides part.

Does the bug relate to non-standard syntax?

No response

Proposal to fix the bug

Not sure exactly and haven't read the source code long enough, but this could be linked to this part:

let augmentedConfig = await augmentConfigBasic(
stylelint,
config,
configDir,
true,
configDir,
filePath,
);

Here we can see that configDir (which is in fact configBasedir) is passed twice to the augmentConfigBasic function.

async function augmentConfigBasic(
stylelint,
config,
configDir,
allowOverrides,
rootConfigDir,
filePath,
) {
let augmentedConfig = config;
if (allowOverrides) {
augmentedConfig = addOptions(stylelint, augmentedConfig);
}
if (filePath) {
augmentedConfig = applyOverrides(augmentedConfig, rootConfigDir, filePath);
}
augmentedConfig = await extendConfig(
stylelint,
augmentedConfig,
configDir,
rootConfigDir,
filePath,
);
const cwd = stylelint._options.cwd;
return absolutizePaths(augmentedConfig, configDir, cwd);
}

Then augmentConfigBasic seems to call applyOverrides with the rootConfigDir parameter which perhaps should be the directory where the config file is and not configBasedir...

@ybiquitous
Copy link
Member

@sronveaux Thanks for the report with the reproduction repository.

The --config-basedir flag indicates using an "absolute path to the directory", but can you resolve the problem by specifying --config-basedir ${your_absolute_path}?

@ybiquitous ybiquitous added the status: needs clarification triage needs clarification from author label Dec 15, 2022
@sronveaux
Copy link
Author

Hi @ybiquitous and thanks for the very fast reply.

I tried as suggested to run with --config-basedir using an absolute path. After that, I'm unable to write the files part in overrides to make it work:

{
  "extends": [
    "stylelint-config-standard"
   ],
  "overrides": [
    {
      "files": ["../*.html", "../**/*.html"],
      "customSyntax": "postcss-html"
    }
  ]
}

Whether I write files": ["../*.html", "../**/*.html"] or files": ["*.html", "**/*.html"], the postcss-css customSyntax isn't applied anymore as this can be confirmed by running stylelint with --print-config.

To clarify a little bit more a concrete use case where this config is used, I have a project where I have a Django backend serving multiple Vue SPA. The whole Django part is inside a backend directory and the Vue parts are inside a frontend directory.
To avoid spreading npm packages and node_modules directories everywhere in the repository, Stylelint is installed inside the frontend directory where npm and Webpack are used.
The Django part has multiple templates which would greatly benefit from stylelint too. So I wrote a general .stylelintrc file at the root of the repository and try to make it lint in both backend and frontend directories. For stylelint to find the packages such as postcss-html, I have to specify the configBasedir to be frontend. Otherwise, I start getting errors such as Could not find "stylelint-config-standard". Do you need a 'configBasedir'?. From there, I can't, inside my stylelintrc file extend some other configs such as stylelint-config-standard-vue as the files part is written as files": ["*.vue", "**/*.vue"] but I have to use files": ["../*.vue", "../**/*.vue"] to step up the directory structure then get down in backend.

That's why I was wondering if it wouldn't be logical that the content of the .stylelintrc file would be applied relative to it instead of relative to the configBasedir which I had to give, not to change the absolute directory where my config starts to apply but only for stylelint to find the base configs which I have to extend.
Perhaps I missed something and have the possibility to give a directory from which config packages such as stylelint-config-standard-vue are available without modifying the directory from which my .stylelintrc file should be applied?

I currently simply installed stylelint at the root of my repository for this to work properly, but this structure is not very tidy and should be avoided.

I should also add that I encounter problems with the VSCode Stylelint extension and didn't manage to find a common config to make both the extension AND the CLI to work with. Perhaps I should also open an issue on the VSCode extension repo too?

@ybiquitous
Copy link
Member

I understand your pain point. However, let me make the reproduction steps clear.

$ git clone https://github.com/sronveaux/stylelint-bug-report
...

$ cd stylelint-bug-report/stylelintInstall

$ npm install
...

$ ./node_modules/.bin/stylelint ../filesToLint/*

../filesToLint/test.css
 1:1   ✖  Expected class selector ".trucBidule" to be kebab-case  selector-class-pattern
 2:24  ✖  Unexpected missing generic font family                  font-family-no-missing-generic-family-keyword
 2:24  ✖  Unexpected duplicate name Roboto                        font-family-no-duplicate-names

../filesToLint/test.html
 10:57  ✖  Unexpected missing generic font family  font-family-no-missing-generic-family-keyword
 10:57  ✖  Unexpected duplicate name Roboto        font-family-no-duplicate-names

5 problems (5 errors, 0 warnings)

Everything works here.
Next, let's move to the project root directory and run stylelint again:

$ cd ..

$ ./stylelintInstall/node_modules/.bin/stylelint ./filesToLint/*
Error: Could not find "stylelint-config-standard". Do you need a `configBasedir`?
...

We get the error, as you commented.
Then, let's specify the --config-basedir flag:

$ ./stylelintInstall/node_modules/.bin/stylelint ./filesToLint/* --config-basedir "$PWD/stylelintInstall"
/Users/koba/tmp/stylelint-bug-report/filesToLint/test.html: When linting something other than CSS, you should install an appropriate syntax, e.g. "postcss-html", and use the "customSyntax" option

filesToLint/test.css
 1:1   ✖  Expected class selector ".trucBidule" to be kebab-case  selector-class-pattern
 2:24  ✖  Unexpected missing generic font family                  font-family-no-missing-generic-family-keyword
 2:24  ✖  Unexpected duplicate name Roboto                        font-family-no-duplicate-names

filesToLint/test.html
 1:1  ✖  Unknown word  CssSyntaxError

4 problems (4 errors, 0 warnings)

Now, an error about "customSyntax": "postcss-html" resolution occurs.

Are the reproduction steps above what you expect?

@sronveaux
Copy link
Author

Hi @ybiquitous and thanks again,

That's exactly the reproduction steps.

To keep it simple, from there, I wonder how I can manage to lint the .html files without changing the .stylelintrc file. Do I miss something somewhere?

@ybiquitous
Copy link
Member

Thanks. I found a bug in the customSyntax resolution logic.

I believe customSyntax or --custom-syntax also should be resolved from configBasedir like plugins or extends, but actually not. This should be a bug.

optionsBase.customSyntax = getModulePath(process.cwd(), cli.flags.customSyntax);

resolved = require(customSyntax);

We need to pass configBasedir to getModulePath() like this:

return getModulePath(configDir, lookup, cwd);

So, I will try writing a patch to fix the bug within a few days.

Or if you are interested in the contribution, please consider contributing.

Thank you so much for the detailed report!

@ybiquitous ybiquitous added status: ready to implement is ready to be worked on by someone type: bug a problem with a feature or rule and removed status: needs clarification triage needs clarification from author labels Dec 15, 2022
@ybiquitous ybiquitous changed the title --config-basedir seems to influence file matching in overrides Fix customSyntax resolution with configBasedir Dec 15, 2022
@sronveaux
Copy link
Author

Many thanks for the fast investigation here!

I'm not 100% sure solving this bug will solve the reported problem but as I've said earlier, I haven't delved in the source code enough to fully understand all the consequences of such a change...

What you've found clearly makes sense and the proposed solution would be more logical for sure ! I'll be very happy if the detailed report helps to make stylelint better!

Thanks again!

@ybiquitous ybiquitous added status: wip is being worked on by someone and removed status: ready to implement is ready to be worked on by someone labels Dec 22, 2022
@ybiquitous
Copy link
Member

@sronveaux I've opened PR #6536 which could fix your problem. If you have time, could you try it? You can install it via:

npm install 'github:stylelint/stylelint#issue-6525'

@sronveaux
Copy link
Author

@ybiquitous thanks for the fast reply and patch, unfortunately, this doesn't solve my problem as I still have to write the overrides part of the config file as

"overrides": [
  {
    "files": ["../*.html", "../**/*.html"],
    "customSyntax": "postcss-html"
  }
]

and writing "files": ["../*.html", "../**/*.html"] instead of "files": ["*.html", "**/*.html"] is the point because I can't extend configs such as stylelint-config-recommended-vue which contains "files": ["*.html", "**/*.html"] inside them.

It seems that what you write inside .stylelintrc is also relative to the configBasedir instead of relative from itself which is the main problem which was described here at first.

It also seems, on my computer, not to fix what you had written above in reproduction steps:

$ ./stylelintInstall/node_modules/.bin/stylelint ./filesToLint/* --config-basedir "$PWD/stylelintInstall"
/Users/koba/tmp/stylelint-bug-report/filesToLint/test.html: When linting something other than CSS, you should install an appropriate syntax, e.g. "postcss-html", and use the "customSyntax" option

filesToLint/test.css
 1:1   ✖  Expected class selector ".trucBidule" to be kebab-case  selector-class-pattern
 2:24  ✖  Unexpected missing generic font family                  font-family-no-missing-generic-family-keyword
 2:24  ✖  Unexpected duplicate name Roboto                        font-family-no-duplicate-names

filesToLint/test.html
 1:1  ✖  Unknown word  CssSyntaxError

4 problems (4 errors, 0 warnings)

I personally still encounter this problem, except if I patch the .stylelintrc file as described above...

@ybiquitous
Copy link
Member

Oh, sorry, my bad. Actually, PR #6536 doesn't fix your problem. The cause is that relative overrides.files paths are resolved from configBasedir, as you pointed out.

stylelint-config-recommended-vue includes files: ["*.vue", "**/*.vue"], so I believe there are the following ways to resolve this:

// .stylelintrc.js
module.exports = {
  overrides: [
    {
      files: ["../*.vue", "../**/*.vue"],
      extends: ["stylelint-config-recommended", "stylelint-config-html"],
      rules: require("stylelint-config-recommended-vue/lib/vue-specific-rules"),
    },
  ],
}
  • Put .stylelintrc and package.json in the same directory (project root). It's the best straightforward.

I would be happy to find another excellent solution, but unfortunately, I can't think of one right now...

@sronveaux
Copy link
Author

@ybiquitous, thanks for the fast reply.

No problem, that's how I'm working for now, having .stylelintrc and package.json at the root of my project.

I personally think there is something to be improved in the future here as the idea of having a single installation of Stylelint used at different places is certainly something I'm not the only one to try having...

In fact, what could be improved is linked to this part of the documentation:

The value of "extends" is a "locater" (or an array of "locaters") that is ultimately require()d. It can fit whatever format works with Node's require.resolve() algorithm. That means a "locater" can be:

* the name of a module in node_modules (e.g. stylelint-config-standard; that module's main file must be a valid JSON configuration)
* an absolute path to a file (which makes sense if you're creating a JS object in a Node.js context and passing it in) with a .js or .json extension.
* a relative path to a file with a .js or .json extension, relative to the referencing configuration (e.g. if configA has extends: "../configB", we'll look for configB relative to configA).

I personally think as the first point says that configs such as stylelint-config-standard should be found directly as they are modules in node_module. If they were, there would be no need to change the configBasedir so the discussed problem would not exist at all... Do you have plans to work on this part in the future ?

Many thanks again for your time, and thanks for sharing such a great project !

Seasons greetings,

@ybiquitous ybiquitous added status: needs discussion triage needs further discussion status: wip is being worked on by someone type: bug a problem with a feature or rule and removed status: wip is being worked on by someone type: bug a problem with a feature or rule status: needs discussion triage needs further discussion labels Dec 24, 2022
@ybiquitous ybiquitous self-assigned this Dec 28, 2022
@ybiquitous ybiquitous changed the title Fix customSyntax resolution with configBasedir Fix overrides.files in config for glob patterns like *.vue Dec 28, 2022
@ybiquitous
Copy link
Member

@sronveaux I've created another PR #6547 to fix the problem of glob patterns like *.vue. Could you please verify the PR version via the following command again?

npm install 'github:stylelint/stylelint#issue-6525'

@sronveaux
Copy link
Author

@ybiquitous thanks again for the fast reply to my last answer.

I just tried PR #6547 and I can tell you that it fixes everything!
What we discussed is working with those changes, in the reproduction repository but also in the real project I encountered the problem in!
Even the problems which were appearing when using the Stylelint VSCode Extension are now resolved with it!

Many thanks again for the time spent on this issue, the result works better than expected! Hope this will be added to the next release!

Thanks for the excellent work and the excellent software which is Stylelint

Cheers,
Sébastien

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: wip is being worked on by someone type: bug a problem with a feature or rule
Development

Successfully merging a pull request may close this issue.

2 participants