Fix incorrect `config` option for Node.js API with `extends` and `overrides` #8030

ybiquitous · 2024-10-09T14:22:58Z

Which issue, if any, is this issue related to?

Closes #8014
(This bug was introduced in #7826)

Is there anything in the PR that needs further explanation?

The bug's root cause is present in caching in the getConfigForFile() function. The current logic depends on the overrides property like this:

stylelint/lib/getConfigForFile.mjs

Lines 31 to 35 in baf7c0e

    
           // If config has overrides the resulting config might be different for some files. 
        
           // Cache results only if resulted config is the same for all linted files. 
        
           if (cached && !optionsConfig.overrides) { 
        
           	return cached; 
        
           }

But, in the case that overrides is only in nested config files, optionsConfig.overrides is falsy, and then an incorrect cached config is returned unexpectedly, for example:

standalone({
  config: {
    extends: './with-overrides.js',
    rules: {},
  },
});

// with-overrides.js
export default {
  overrides: [ 
    { 
      files: ["*.css"],
      rules: {
        'block-no-empty': true,
      },
    },
  ],
};

So, this PR uses optionsConfig and filePath for caching keys, instead of optionsConfig.overrides. For that purpose, we have to change the structure of the cache data like this:

- Map<Config, Promise<CosmiconfigResult>>
+ Map<Config, Map<string, CosmiconfigResult>>

ybiquitous · 2024-10-09T14:24:26Z

lib/__tests__/overrides.test.mjs

+		const { results } = await standalone({
+			files: [path.join(fixturesPath, 'style.css')],
+			config: {
+				extends: path.join(fixturesPath, 'extends-in-overrides.json'),


[note] Here's the content of extends-in-overrides.json:

stylelint/lib/__tests__/fixtures/config-overrides/extends-in-overrides.json

Lines 1 to 8 in baf7c0e

{

"overrides": [

{

"files": ["*.css"],

"extends": "./extending-simple-rule.json"

}

]

}

And extending-simple-rule.json:

stylelint/lib/__tests__/fixtures/config-overrides/extending-simple-rule.json

Lines 1 to 14 in baf7c0e

{

"extends": "./simple-rule",

"rules": {

"block-no-empty": true

},

"overrides": [

{

"files": ["*.css"],

"rules": {

"color-named": "never"

}

}

]

}

romainmenke

This looks great @ybiquitous
Thank your for investigating and finding a solution 🙇

ybiquitous · 2024-10-09T15:46:34Z

@romainmenke Thanks for the quick review!

I just triggered the workflow in the ecosystem test, so let's see the result!
https://github.com/stylelint/stylelint-ecosystem-tester/actions/runs/11258475686

EDIT: The test is now passed! 😄

romainmenke · 2024-10-09T16:12:36Z

Awesome! Thank you for working on this bug!

Mouvedia · 2024-10-09T18:49:37Z

lib/getConfigForFile.mjs

-	if (optionsConfig !== undefined) {
-		const cached = stylelint._specifiedConfigCache.get(optionsConfig);
+	if (optionsConfig) {
+		const filePathAsCacheKey = filePath ?? '';


question

Is it safe as a fallback cache key?
i.e. I would have expected a test in getConfigForFile.test.mjs with filePath === undefined.

I am not sure if cachedForFiles.set(filePathAsCacheKey, augmentedResult); should be set if filePathAsCacheKey === ''.

Yes, it's safe. Such a test isn't always necessary since getConfigForFile() calls without filePath are already performed at various places. Other higher-level test cases cover. Of course, I'm not against having a case in getConfigForFile.test.mjs too.

https://github.com/search?q=repo%3Astylelint%2Fstylelint+%22getConfigForFile%28%22+path%3A*.mjs&type=code

We can see also the coverage passes the line of cachedForFiles.set():

https://app.codecov.io/gh/stylelint/stylelint/commit/3fca68706b744b18df53a2691dbd164508cd5d32/blob/lib/getConfigForFile.mjs

If you are sure that we can't have a case where we get the wrong cached from cachedForFiles.get('') because filePath was undefined more than once for getConfigForFile calls with different stylelint._options.config, then we can move on.
I would need to investigate further to tell if it can actually happen.
It seemed unsafe to me at first glance.

Mouvedia · 2024-10-11T21:32:10Z

Since it is now using filePath instead of stylelint._options.config as the key, it means that the
const postcssResult = await lintSource(stylelint, { in the const getStylelintResults = absoluteFilePaths.map(async (absoluteFilepath) => { loop of standalone.mjs won't get the cached "augmented" config for the second file and forward because we get a new filePathAsCacheKey for each iteration.
i.e. it will run const augmentedResult = await augmentConfigFull(stylelint, … and skip if (cached) { which wasn't the original purpose of that if IMO

stylelint/lib/lintSource.mjs

Line 60 in c360343

    
           configForFile = await getConfigForFile(stylelint, configSearchPath, inputFilePath);

What I am getting at is that 16.10.0 is probably slower than 16.9.0 if you have pass more than one file as CLI parameters.
I think the cache key should be something common to all the files being passed.

@ybiquitous Am I correct? Or am I missing something obvious?

@Mouvedia

| datasource | package | from | to | | ---------- | --------- | ------ | ------- | | npm | stylelint | 16.9.0 | 16.10.0 | ## [v16.10.0](https://github.com/stylelint/stylelint/blob/HEAD/CHANGELOG.md#16100) - Changed: `shorthand-property-no-redundant-values` reported ranges ([#8008](stylelint/stylelint#8008)) ([@Mouvedia](https://github.com/Mouvedia)). - Added: `formatter` configuration property ([#7826](stylelint/stylelint#7826)) ([@emmacharp](https://github.com/emmacharp)). - Added: `validate` option to Node.js API and `--validate` CLI flag ([#8009](stylelint/stylelint#8009)) ([@Mouvedia](https://github.com/Mouvedia)). - Added: fixed section to the output of the `verbose` formatter ([#7985](stylelint/stylelint#7985)) ([@Mouvedia](https://github.com/Mouvedia)). - Added: support for `string | RegExp` to `ignoreValues` option of `value-no-vendor-prefix` ([#7969](stylelint/stylelint#7969)) ([@Mouvedia](https://github.com/Mouvedia)). - Added: support for `string` to `ignoreLonghands` option of `declaration-block-no-redundant-longhand-properties` ([#7971](stylelint/stylelint#7971)) ([@Mouvedia](https://github.com/Mouvedia)). - Fixed: `at-rule-no-unknown` false positives for `@position-try` ([#7968](stylelint/stylelint#7968)) ([@yelizsevinc](https://github.com/yelizsevinc)). - Fixed: `declaration-block-no-redundant-longhand-properties` autofix for `grid-column`/`grid-row` ([#8023](stylelint/stylelint#8023)) ([@ryo-manba](https://github.com/ryo-manba)). - Fixed: `declaration-property-value-no-unknown` false negatives/positives via [`[email protected]`](https://github.com/csstree/csstree/releases/tag/v3.0.0) ([#7993](stylelint/stylelint#7993)) ([@Mouvedia](https://github.com/Mouvedia)). - Fixed: `declaration-property-value-no-unknown` reported ranges and message ([#8003](stylelint/stylelint#8003)) ([@romainmenke](https://github.com/romainmenke)). - Fixed: `keyframe-selector-notation` reported ranges ([#7992](stylelint/stylelint#7992)) ([@romainmenke](https://github.com/romainmenke)). - Fixed: `length-zero-no-unit` false positives for Sass variables ([#8035](stylelint/stylelint#8035)) ([@ryo-manba](https://github.com/ryo-manba)). - Fixed: `no-descending-specificity` reported ranges ([#7996](stylelint/stylelint#7996)) ([@romainmenke](https://github.com/romainmenke)). - Fixed: `selector-anb-no-unmatchable` reported ranges and message ([#8001](stylelint/stylelint#8001)) ([@romainmenke](https://github.com/romainmenke)). - Fixed: `string` formatter handling of log symbols for non-Unicode terminals ([#7981](stylelint/stylelint#7981)) ([@Mouvedia](https://github.com/Mouvedia)). - Fixed: built `.cjs` files to address Rollup vulnerability ([#8010](stylelint/stylelint#8010)) ([@ybiquitous](https://github.com/ybiquitous)). - Fixed: incorrect `config` option for Node.js API with `extends` and `overrides` ([#8030](stylelint/stylelint#8030)) ([@ybiquitous](https://github.com/ybiquitous)). - Fixed: stricter validation for the options of `declaration-property-max-values`, `declaration-property-value-no-unknown` and `number-max-precision` ([#7975](stylelint/stylelint#7975)) ([@Mouvedia](https://github.com/Mouvedia)).

@Mouvedia

| datasource | package | from | to | | ---------- | --------- | ------ | ------- | | npm | stylelint | 16.9.0 | 16.10.0 | ## [v16.10.0](https://github.com/stylelint/stylelint/blob/HEAD/CHANGELOG.md#16100) - Changed: `shorthand-property-no-redundant-values` reported ranges ([#8008](stylelint/stylelint#8008)) ([@Mouvedia](https://github.com/Mouvedia)). - Added: `formatter` configuration property ([#7826](stylelint/stylelint#7826)) ([@emmacharp](https://github.com/emmacharp)). - Added: `validate` option to Node.js API and `--validate` CLI flag ([#8009](stylelint/stylelint#8009)) ([@Mouvedia](https://github.com/Mouvedia)). - Added: fixed section to the output of the `verbose` formatter ([#7985](stylelint/stylelint#7985)) ([@Mouvedia](https://github.com/Mouvedia)). - Added: support for `string | RegExp` to `ignoreValues` option of `value-no-vendor-prefix` ([#7969](stylelint/stylelint#7969)) ([@Mouvedia](https://github.com/Mouvedia)). - Added: support for `string` to `ignoreLonghands` option of `declaration-block-no-redundant-longhand-properties` ([#7971](stylelint/stylelint#7971)) ([@Mouvedia](https://github.com/Mouvedia)). - Fixed: `at-rule-no-unknown` false positives for `@position-try` ([#7968](stylelint/stylelint#7968)) ([@yelizsevinc](https://github.com/yelizsevinc)). - Fixed: `declaration-block-no-redundant-longhand-properties` autofix for `grid-column`/`grid-row` ([#8023](stylelint/stylelint#8023)) ([@ryo-manba](https://github.com/ryo-manba)). - Fixed: `declaration-property-value-no-unknown` false negatives/positives via [`[email protected]`](https://github.com/csstree/csstree/releases/tag/v3.0.0) ([#7993](stylelint/stylelint#7993)) ([@Mouvedia](https://github.com/Mouvedia)). - Fixed: `declaration-property-value-no-unknown` reported ranges and message ([#8003](stylelint/stylelint#8003)) ([@romainmenke](https://github.com/romainmenke)). - Fixed: `keyframe-selector-notation` reported ranges ([#7992](stylelint/stylelint#7992)) ([@romainmenke](https://github.com/romainmenke)). - Fixed: `length-zero-no-unit` false positives for Sass variables ([#8035](stylelint/stylelint#8035)) ([@ryo-manba](https://github.com/ryo-manba)). - Fixed: `no-descending-specificity` reported ranges ([#7996](stylelint/stylelint#7996)) ([@romainmenke](https://github.com/romainmenke)). - Fixed: `selector-anb-no-unmatchable` reported ranges and message ([#8001](stylelint/stylelint#8001)) ([@romainmenke](https://github.com/romainmenke)). - Fixed: `string` formatter handling of log symbols for non-Unicode terminals ([#7981](stylelint/stylelint#7981)) ([@Mouvedia](https://github.com/Mouvedia)). - Fixed: built `.cjs` files to address Rollup vulnerability ([#8010](stylelint/stylelint#8010)) ([@ybiquitous](https://github.com/ybiquitous)). - Fixed: incorrect `config` option for Node.js API with `extends` and `overrides` ([#8030](stylelint/stylelint#8030)) ([@ybiquitous](https://github.com/ybiquitous)). - Fixed: stricter validation for the options of `declaration-property-max-values`, `declaration-property-value-no-unknown` and `number-max-precision` ([#7975](stylelint/stylelint#7975)) ([@Mouvedia](https://github.com/Mouvedia)).

michwads · 2024-10-14T18:21:15Z

We are using stylelint with our vite project — and something in 16.10.0 is causing errors for us: "Internal server error: Cannot read properties of undefined (reading 'config')"

Current setup is that we have a .stylelintrc.yml that extends our global .stylelintrc.json; but we keep getting this error even though 16.9.0 works just fine! This is the only part of the changelog referencing a config > so starting here.

ybiquitous · 2024-10-16T00:54:11Z

@michwads @romainmenke Thanks for the report. I'll look into #8044 when I have time.

ybiquitous · 2024-10-16T00:58:40Z

What I am getting at is that 16.10.0 is probably slower than 16.9.0 if you have pass more than one file as CLI parameters.
I think the cache key should be something common to all the files being passed.

@Mouvedia Yes, it's possible to slow down. But I think it's unavoidable to return a correct config because loading configs depends on file paths by cascading.

And, I think its impact is limited because there are fewer cases to use the config property, instead of config files.

If you're interested in benchmarks, could you open a new issue about the topic, instead of continuing the discussion in this closed PR?

Add test case to reproduce bug for overrides config property

65bc754

This comment was marked as resolved.

Sign in to view

ybiquitous commented Oct 9, 2024

View reviewed changes

Fix caching in getConfigForFile() when config object is given

7c98534

ybiquitous changed the title ~~Fix config option for Node.js API with extends and overrides~~ Fix incorrect config option for Node.js API with extends and overrides Oct 9, 2024

Add changelog

f2c6b7e

ybiquitous marked this pull request as ready for review October 9, 2024 14:58

ybiquitous mentioned this pull request Oct 9, 2024

Fix incorrect config when using standalone Stylelint #8014

Closed

romainmenke approved these changes Oct 9, 2024

View reviewed changes

ybiquitous merged commit ab2141e into main Oct 9, 2024
17 checks passed

ybiquitous deleted the issue-8014 branch October 9, 2024 15:44

Mouvedia reviewed Oct 9, 2024

View reviewed changes

This comment was marked as resolved.

Sign in to view

romainmenke mentioned this pull request Oct 14, 2024

Fix unexpected cannot read properties of undefined error when reading config #8044

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix incorrect `config` option for Node.js API with `extends` and `overrides` #8030

Fix incorrect `config` option for Node.js API with `extends` and `overrides` #8030

ybiquitous commented Oct 9, 2024 •

edited

Loading

This comment was marked as resolved.

ybiquitous Oct 9, 2024 •

edited

Loading

romainmenke left a comment

ybiquitous commented Oct 9, 2024 •

edited

Loading

romainmenke commented Oct 9, 2024

Mouvedia Oct 9, 2024

ybiquitous Oct 9, 2024 •

edited

Loading

Mouvedia Oct 10, 2024 •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

Mouvedia commented Oct 11, 2024 •

edited

Loading

michwads commented Oct 14, 2024 •

edited

Loading

ybiquitous commented Oct 16, 2024

ybiquitous commented Oct 16, 2024

	// If config has overrides the resulting config might be different for some files.
	// Cache results only if resulted config is the same for all linted files.
	if (cached && !optionsConfig.overrides) {
	return cached;
	}

	{
	"overrides": [
	{
	"files": ["*.css"],
	"extends": "./extending-simple-rule.json"
	}
	]
	}

	{
	"extends": "./simple-rule",
	"rules": {
	"block-no-empty": true
	},
	"overrides": [
	{
	"files": ["*.css"],
	"rules": {
	"color-named": "never"
	}
	}
	]
	}

Fix incorrect config option for Node.js API with extends and overrides #8030

Fix incorrect config option for Node.js API with extends and overrides #8030

Conversation

ybiquitous commented Oct 9, 2024 • edited Loading

This comment was marked as resolved.

ybiquitous Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

romainmenke left a comment

Choose a reason for hiding this comment

ybiquitous commented Oct 9, 2024 • edited Loading

romainmenke commented Oct 9, 2024

Mouvedia Oct 9, 2024

Choose a reason for hiding this comment

ybiquitous Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Mouvedia Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Mouvedia commented Oct 11, 2024 • edited Loading

michwads commented Oct 14, 2024 • edited Loading

ybiquitous commented Oct 16, 2024

ybiquitous commented Oct 16, 2024

Fix incorrect `config` option for Node.js API with `extends` and `overrides` #8030

Fix incorrect `config` option for Node.js API with `extends` and `overrides` #8030

ybiquitous commented Oct 9, 2024 •

edited

Loading

ybiquitous Oct 9, 2024 •

edited

Loading

ybiquitous commented Oct 9, 2024 •

edited

Loading

ybiquitous Oct 9, 2024 •

edited

Loading

Mouvedia Oct 10, 2024 •

edited

Loading

Mouvedia commented Oct 11, 2024 •

edited

Loading

michwads commented Oct 14, 2024 •

edited

Loading