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

Add no-invalid-position-at-import-rule #5202

Merged
merged 10 commits into from Mar 26, 2021
1 change: 1 addition & 0 deletions docs/user-guide/rules/list.md
Expand Up @@ -79,6 +79,7 @@ Grouped first by the following categories and then by the [_thing_](http://apps.
- [`no-empty-source`](../../../lib/rules/no-empty-source/README.md): Disallow empty sources.
- [`no-extra-semicolons`](../../../lib/rules/no-extra-semicolons/README.md): Disallow extra semicolons (Autofixable).
- [`no-invalid-double-slash-comments`](../../../lib/rules/no-invalid-double-slash-comments/README.md): Disallow double-slash comments (`//...`) which are not supported by CSS.
- [`no-invalid-position-at-import-rule`](../../../lib/rules/no-invalid-position-at-import-rule/README.md): Disallow invalid position @import rules within a stylesheet.
chuuddo marked this conversation as resolved.
Show resolved Hide resolved

## Limit language features

Expand Down
3 changes: 3 additions & 0 deletions lib/rules/index.js
Expand Up @@ -242,6 +242,9 @@ const rules = {
'no-invalid-double-slash-comments': importLazy(() =>
require('./no-invalid-double-slash-comments'),
)(),
'no-invalid-position-at-import-rule': importLazy(() =>
require('./no-invalid-position-at-import-rule'),
)(),
'no-missing-end-of-source-newline': importLazy(() =>
require('./no-missing-end-of-source-newline'),
)(),
Expand Down
49 changes: 49 additions & 0 deletions lib/rules/no-invalid-position-at-import-rule/README.md
@@ -0,0 +1,49 @@
# no-invalid-position-at-import-rule

Disallow invalid position `@import` rules within a stylesheet.

<!-- prettier-ignore -->
```css
a {}
@import 'foo.css';
/** ↑
* Invalid position */
```
chuuddo marked this conversation as resolved.
Show resolved Hide resolved

chuuddo marked this conversation as resolved.
Show resolved Hide resolved
## Options

### `true`

The following patterns are considered violations:

<!-- prettier-ignore -->
```css
a {}
@import 'foo.css';
```

<!-- prettier-ignore -->
```css
@media print {}
@import 'foo.css';
```

The following patterns are _not_ considered violations:

<!-- prettier-ignore -->
```css
@import 'foo.css';
a {}
```

<!-- prettier-ignore -->
```css
/* some comment */
@import 'foo.css';
```

<!-- prettier-ignore -->
```css
@charset 'utf-8';
@import 'foo.css';
```
80 changes: 80 additions & 0 deletions lib/rules/no-invalid-position-at-import-rule/__tests__/index.js
@@ -0,0 +1,80 @@
'use strict';

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

testRule({
ruleName,
config: [true],

accept: [
{
code: `
chuuddo marked this conversation as resolved.
Show resolved Hide resolved
@import 'foo.css';
a {}
`,
description: '@import on first line',
},
{
code: `
/* some comment */
@import 'foo.css';
`,
description: '@import after comment',
},
{
code: `
@charset 'utf-8';
@import 'foo.css';
`,
description: '@import after @charset ',
},
{
code: `
@import 'foo.css';
@import 'bar.css';
`,
description: '@import after another @import',
},
{
code: `
@CHARSET 'utf-8';
@imPORT 'foo.css';
@import 'bar.css';
`,
description: 'case insensitive',
},
],

reject: [
chuuddo marked this conversation as resolved.
Show resolved Hide resolved
{
code: `
a {}
@import 'foo.css';
`,
message: messages.rejected('foo.css'),
description: '@import after selector',
line: 3,
column: 4,
},
{
code: `
@media print {}
@import url('foo.css');
`,
message: messages.rejected('foo.css'),
description: '@import after another at-rule',
line: 3,
column: 4,
},
{
code: `
@media print {}
@imPort URl('foo.css');
`,
message: messages.rejected('foo.css'),
description: 'case insensitive',
line: 3,
column: 4,
},
chuuddo marked this conversation as resolved.
Show resolved Hide resolved
],
});
67 changes: 67 additions & 0 deletions lib/rules/no-invalid-position-at-import-rule/index.js
@@ -0,0 +1,67 @@
// @ts-nocheck

'use strict';

const report = require('../../utils/report');
const ruleMessages = require('../../utils/ruleMessages');
const validateOptions = require('../../utils/validateOptions');
const valueParser = require('postcss-value-parser');

const ruleName = 'no-invalid-position-at-import-rule';

const messages = ruleMessages(ruleName, {
rejected: (atImport) => `Unexpected invalid position @import rule ${atImport}`,
chuuddo marked this conversation as resolved.
Show resolved Hide resolved
});

function rule(actual) {
return (root, result) => {
const validOptions = validateOptions(result, ruleName, { actual });

if (!validOptions) {
return;
}

let invalidPosition = false;

root.walk((node) => {
if (node.type === 'comment' || (node.type === 'atrule' && /^charset$/i.test(node.name))) {
return;
}

if (node.type === 'atrule' && /^import$/i.test(node.name)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[IMO] It seems a bit readable to me if using toLowerCase. But this is just my opinion, so you can ignore it. 😃

Suggested change
if (node.type === 'comment' || (node.type === 'atrule' && /^charset$/i.test(node.name))) {
return;
}
if (node.type === 'atrule' && /^import$/i.test(node.name)) {
const nodeName = node.name.toLowerCase();
if (node.type === 'comment' || (node.type === 'atrule' && nodeName === 'charset')) {
return;
}
if (node.type === 'atrule' && nodeName === 'import') {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion looks suitable. Just made some research and toLowerCase() used much more often in rules.
@jeddy3 what do you think about it? Do we have any convention on this or should create one if not?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the same thought about toLowerCase as @ybiquitous when I was first reviewing the pull request as we've used that approach in loads of places. However, the regex approach sort of grew on me because we use it for filtering at-rules and declarations when walking (like we did in the last rule we added), so I didn't mention it.

Do we have any convention on this or should create one if not?

I'm all for conventions, especially within rules, because they're such a time saver when reviewing code.

I'm not sure which of the approaches is more readable, though. So, let's go with @ybiquitous's toLowerCase suggestion for now to be consistent with how we're treating names and values in other rules. We can then, if we want, refactor across the board later down the line.

Copy link
Member

@jeddy3 jeddy3 Mar 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following on, we'll probably stick with toLowerCase (for names and values) because (having looked myself) we often:

  • do more string manipulation at the same time, e.g. trimming and slicing
  • look for matches in arrays and sets

if (invalidPosition) {
const url = getAtImportUrl(node);

if (url) {
chuuddo marked this conversation as resolved.
Show resolved Hide resolved
report({
message: messages.rejected(url),
node,
result,
ruleName,
});
}
}

return;
}

invalidPosition = true;
});
};
}

function getAtImportUrl(node) {
const params = valueParser(node.params).nodes;

if (!params.length) {
return null;
}

return params[0].type === 'function' && /^url$/i.test(params[0].value)
? params[0].nodes[0].value
: params[0].value;
}

rule.ruleName = ruleName;
rule.messages = messages;
module.exports = rule;