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 no-duplicate-at-import-rules false negatives for imports with supports and layer conditions #6997

Closed
romainmenke opened this issue Jun 28, 2023 · 2 comments · Fixed by #7001
Labels
status: wip is being worked on by someone type: bug a problem with a feature or rule

Comments

@romainmenke
Copy link
Member

What minimal example or steps are needed to reproduce the bug?

@import url('foo.css') layer supports(display: grid) tv, screen;
@import url('foo.css') layer supports(display: grid) screen;

What minimal configuration is needed to reproduce the bug?

{
  "extends": [
    "stylelint-config-standard"
  ]
}

How did you run Stylelint?

https://stylelint.io/demo/#N4Igxg9gJgpiBcIACBLAtgBwgJwC4AIBXbAGwAoByAMwggDowBnRigSnxIEMBPGbfRoQxY8jMlBSMMXbvHwBzbCijtcANwA0AsNhgwAdgG4AOvtSYcBYuWq0GzNhx58BQkbjESpMuYuXtGHT0jEA0QKhQSGAA5TjQ4RBgADzjpGHtGUPAIfQj5BBBgU3x8YxBk3AMoRjK5AG1iktKQRlxuKJIUfVwAWkhclHke1s59KE5sKDLGgF1TAF8s-ryAMRw0TlwCgCtGHKzYDEzEIv0Sstb2mE7u2uauStayjUaLto6u3uXB4dxR8cmdzKDxgTxACxA8yAA

Which Stylelint-related dependencies are you using?

{
  "stylelint": "latest",
  "stylelint-config-standard": "latest"
}

What did you expect to happen?

I expected an error :

Unexpected duplicate @import rule foo.css [(no-duplicate-at-import-rules)](https://stylelint.io/user-guide/rules/no-duplicate-at-import-rules)[2:1-60]

What actually happened?

No error was reported

Do you have a proposal to fix the bug?

I propose that we consider layer and supports conditions to be part of the cache key.

see : https://github.com/stylelint/stylelint/pull/6996/files#diff-6e755a443d64a1885d2445c671076e00a6a54ebd01eb205b97d7dd21fe358e63R79-R141

@romainmenke
Copy link
Member Author

romainmenke commented Jun 28, 2023

When working on #6848 I found that no-duplicate-at-import-rules does not support the latest syntax.

https://www.w3.org/TR/css-cascade-5/#at-ruledef-import

@import [ <url> | <string> ]
        [ layer | layer(<layer-name>) ]?
        [ supports( [ <supports-condition> | <declaration> ] ) ]?
        <media-query-list>? ;

I suggest that we continue to split the list of media queries but add layer and supports conditions as a prefix to each individual media query.

@import url("a.css") layer(base) supports(display: grid) screen, tv;

has keys :

  • layer(base) supports(display: grid) screen
  • layer(base) supports(display: grid) tv
Proposed code change
/**
 * List the import conditions found in the prelude of an `@import` rule
 *
 * @param {import('postcss-value-parser').Node[]} params
 * @returns {Array<string>}
 */
function listImportConditions(params) {
	if (!params.length) return [];

	/** @type {Array<import('postcss-value-parser').Node>} */
	let layerOrSupportConditions = [];

	/** @type {Array<Array<import('postcss-value-parser').Node>>} */
	let media = [];
	/** @type {Array<import('postcss-value-parser').Node>} */
	let lastMediaQuery = [];

	for (let i = 0; i < params.length; i++) {
		const param = params[i];

		if (!param) break;

		// remove whitespace to get a consistent key
		if (param.type === 'space') {
			continue;
		}

		// remove comments to get a consistent key
		if (param.type === 'comment') {
			continue;
		}

		if (!media.length) {
			// @import url(...) layer(base) supports(display: flex)
			if (param.type === 'function' && (param.value === 'supports' || param.value === 'layer')) {
				layerOrSupportConditions.push(param);
				continue;
			}

			// @import url(...) layer
			if (param.type === 'word' && param.value === 'layer') {
				layerOrSupportConditions.push(param);
				continue;
			}
		}

		if (param.type === 'div' && param.value === ',') {
			media.push(lastMediaQuery);
			lastMediaQuery = [];
			continue;
		}

		lastMediaQuery.push(param);
	}

	if (lastMediaQuery.length) {
		media.push(lastMediaQuery);
	}

	const layerOrSupportConditionsString = layerOrSupportConditions.length
		? `${layerOrSupportConditions.map((c) => valueParser.stringify(c)).join(' ')} `
		: '';

	return media
		.map((m) => {
			return layerOrSupportConditionsString + valueParser.stringify(m).trim();
		})
		.filter((n) => n.length);
}
Extra test coverage
'use strict';

const { messages, ruleName } = require('..');

testRule({
	ruleName,
	config: [true],

	accept: [
		{
			code: '@import "a.css";',
		},
		{
			code: "@import url('a.css');",
		},
		{
			code: '@import url(a.css);',
		},
		{
			code: '@import url("a.css") projection, tv;',
		},
		{
			code: "@import 'a.css'; @import 'b.css';",
		},
		{
			code: "@import url('a.css') projection; @import url('a.css') tv;",
		},
		{
			code: '@import "a.css" screen; @import "b.css" tv; @import "a.css" tv;',
		},
		{
			code: '@IMPORT "a.css"; @ImPoRt "b.css";',
		},
		{
			code: '@import "a.css" supports(display: flex) tv; @import "a.css" layer tv;',
		},
		{
			code: '@import "a.css" supports(display: flex) tv; @import "a.css" supports(display: grid) tv;',
		},
	],

	reject: [
		{
			code: "@import 'a.css'; @import 'a.css';",
			message: messages.rejected(`a.css`),
			line: 1,
			column: 18,
			endLine: 1,
			endColumn: 33,
		},
		{
			code: '@import url("a.css"); @import url("a.css");',
			message: messages.rejected(`a.css`),
			line: 1,
			column: 23,
			endLine: 1,
			endColumn: 43,
		},
		{
			code: '@import "a.css";\n@import \'a.css\';',
			message: messages.rejected(`a.css`),
			line: 2,
			column: 1,
			endLine: 2,
			endColumn: 16,
		},
		{
			code: '@import "a.css"; @import \'b.css\'; @import url(a.css);',
			message: messages.rejected(`a.css`),
			line: 1,
			column: 35,
			endLine: 1,
			endColumn: 53,
		},
		{
			code: "@import url('a.css') tv; @import 'a.css' tv;",
			message: messages.rejected(`a.css`),
			line: 1,
			column: 26,
		},
		{
			code: "@import url('a.css') tv, projection; @import 'a.css' projection, tv;",
			message: messages.rejected(`a.css`),
			line: 1,
			column: 38,
		},
		{
			code: "@import url('a.css') tv, projection; @import 'a.css' projection, screen, tv;",
			message: messages.rejected(`a.css`),
			line: 1,
			column: 38,
		},
		{
			code: "@import url('a.css') tv, projection; @import 'a.css' screen, tv;",
			message: messages.rejected(`a.css`),
			line: 1,
			column: 38,
		},
		{
			code: "@import url('a.css') /* a comment */ tv; @import 'a.css' tv /* a comment */;",
			message: messages.rejected(`a.css`),
			line: 1,
			column: 42,
		},
		{
			code: '@import "a.css" tv, (min-width : 500px);@import url(a.css) (  min-width:500px   ), tv;',
			message: messages.rejected(`a.css`),
			line: 1,
			column: 41,
		},
		{
			code: "@IMPORT 'a.css'; @ImPoRt 'a.css';",
			message: messages.rejected(`a.css`),
			line: 1,
			column: 18,
		},
		{
			code: '@import url("a.css") layer; @import url("a.css") layer;',
			message: messages.rejected(`a.css`),
			line: 1,
			column: 29,
			endLine: 1,
			endColumn: 55,
		},
		{
			code: '@import url("a.css") layer(base); @import url("a.css") layer(base);',
			message: messages.rejected(`a.css`),
			line: 1,
			column: 35,
			endLine: 1,
			endColumn: 67,
		},
		{
			code: '@import url("a.css") layer(base) supports(display: grid); @import url("a.css") layer(base) supports(display: grid);',
			message: messages.rejected(`a.css`),
			line: 1,
			column: 59,
			endLine: 1,
			endColumn: 115,
		},
		{
			code: '@import url("a.css") supports(display: grid); @import url("a.css") supports(display: grid);',
			message: messages.rejected(`a.css`),
			line: 1,
			column: 47,
			endLine: 1,
			endColumn: 91,
		},
		{
			code: '@import url("a.css") layer tv; @import url("a.css") layer tv;',
			message: messages.rejected(`a.css`),
			line: 1,
			column: 32,
			endLine: 1,
			endColumn: 61,
		},
		{
			code: '@import url("a.css") layer(base) tv; @import url("a.css") layer(base) tv;',
			message: messages.rejected(`a.css`),
			line: 1,
			column: 38,
			endLine: 1,
			endColumn: 73,
		},
		{
			code: '@import url("a.css") layer(base) supports(display: grid) tv; @import url("a.css") layer(base) supports(display: grid) tv;',
			message: messages.rejected(`a.css`),
			line: 1,
			column: 62,
			endLine: 1,
			endColumn: 121,
		},
		{
			code: '@import url("a.css") layer(base) supports(display: grid) screen, tv; @import url("a.css") layer(base) supports(display: grid) tv;',
			message: messages.rejected(`a.css`),
			line: 1,
			column: 70,
			endLine: 1,
			endColumn: 129,
		},
	],
});

@romainmenke romainmenke added the status: needs discussion triage needs further discussion label Jun 28, 2023
@jeddy3 jeddy3 changed the title Fix false negatives for no-duplicate-at-import-rules with supports and layer conditions Fix no-duplicate-at-import-rules false negatives for imports with supports and layer conditions Jun 28, 2023
@jeddy3
Copy link
Member

jeddy3 commented Jun 28, 2023

Thank you for the detailed write-up. The proposal SGTM!

@jeddy3 jeddy3 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 discussion triage needs further discussion labels Jun 28, 2023
@romainmenke romainmenke added status: wip is being worked on by someone and removed status: ready to implement is ready to be worked on by someone labels Jun 29, 2023
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
2 participants