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-duplicate-at-import-rules first draft #2963

Merged
merged 11 commits into from Nov 4, 2017

Conversation

7 participants
@agatac
Contributor

agatac commented Oct 14, 2017

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

#2954

Is there anything in the PR that needs further explanation?

I don't think so. It's a first draft, I could use some guidelines how to improve the code.

@agatac agatac changed the title from add no-duplicate-at-import-rules first draft to [WIP] add no-duplicate-at-import-rules first draft Oct 14, 2017

@jeddy3

jeddy3 requested changes Oct 14, 2017 edited

@agatac This is a very good start, thanks!

Similar to how the font-family-no-duplicate-names rule works, I believe this rule should know that @import "a.css", "@import 'a.css', @import url(a.css), @import url("a.css") and @import url('a.css') are all duplicates.

Can you adapt the rule (and add tests) for this please? Having a look at how postcss-import uses postcss-value-parser to parse @import statements might be useful here e.g. to parse out the following:

  1. the filename to import, so you check for dupes regardless of the format e.g. "a.css", "url(a.css)" etc.
  2. the media query list (if there is one), so you can check that imports with the same filename also have the same media query list.
@@ -0,0 +1,44 @@
# no-duplicate-at-import-rules
Disallow duplicate @import rules within a stylesheet.

This comment has been minimized.

@jeddy3

jeddy3 Oct 14, 2017

Member

Disallow duplicate @import rules within a stylesheet.

The following patterns are considered violations:
```css

This comment has been minimized.

@jeddy3

jeddy3 Oct 14, 2017

Member

Please use a.css and b.css in examples and tests as that's a simple convention we've used elsewhere.

The following patterns are considered violations:
```css
@import "mystyle.css";

This comment has been minimized.

@jeddy3

jeddy3 Oct 14, 2017

Member
@import "a.css";
@import "a.css";
@import "mystyle.css";
```
```css

This comment has been minimized.

@jeddy3

jeddy3 Oct 14, 2017

Member
@import "a.css";
@import 'a.css';
```
```css
@import url("mystyle.css");

This comment has been minimized.

@jeddy3

jeddy3 Oct 14, 2017

Member
@import "a.css";
@import 'b.css';
@import url(a.css);
The following patterns are *not* considered violations:
```css
@import "mystyle.css";

This comment has been minimized.

@jeddy3

jeddy3 Oct 14, 2017

Member
@import "a.css";
@import "b.css";
reject: [
{
code: '@import "mystyle.css"; @import "mystyle.css";',
message: messages.rejected(`"mystyle.css"`)

This comment has been minimized.

@jeddy3

jeddy3 Oct 14, 2017

Member

Please include column and line numbers for rejected tests.

{
code: '@import "mystyle.css"; @import "otherstyle.css";'
}
],

This comment has been minimized.

@jeddy3

jeddy3 Oct 14, 2017

Member

Let's add a test for:

@import url("a.css") projection;
@import url("a.css") tv;

as I believe it's technically valid:

When the same style sheet is imported or linked to a document in multiple places, user agents must process (or act as though they do) each link as though the link were to an independent style sheet.

accept: [
{
code: '@import "mystyle.css";'

This comment has been minimized.

@jeddy3

jeddy3 Oct 14, 2017

Member

As with the examples, please use a.css, b.css, c.css in tests etc.

},
{
code:
'@import url("mystyle.css"); a { color: red }; @import url("mystyle.css");',

This comment has been minimized.

@jeddy3

jeddy3 Oct 14, 2017

Member

I think we can remove this test as it's invalid CSS:

Any @import rules must precede all other at-rules and style rules in a style sheet (besides @charset, which must be the first thing in the style sheet if it exists), or else the @import rule is invalid.

@agatac

This comment has been minimized.

Contributor

agatac commented Oct 15, 2017

Hey, how about now? :)

@jeddy3

@agatac Thanks for making the changes. I believe the rule will be even more useful now.

The implementation (and tests) so far have just made me realise we can (although, I'm not sure if we should) take the media query list parsing further e.g. the following are technically duplicates:

@import "a.css" tv, projection;
@import "a.css" projection, tv;

@agatac & @stylelint/core I believe making use of postcss-media-query-parser will enable the extraction (and correct comparison) of the various media features within the media query list. The question is, is it worth the effort to implement it (at this stage) for an arguably seldom used aspect of @imports?

column: 23
},
{
code: "@import \"a.css\"; @import 'a.css';",

This comment has been minimized.

@jeddy3

jeddy3 Oct 15, 2017

Member

Can you please slightly adapt this to:

@import \"a.css\";\n@import 'a.css';

To ensure the line numbering is correct.

message: messages.rejected(`a.css`),
line: 1,
column: 35
}

This comment has been minimized.

@jeddy3

jeddy3 Oct 15, 2017

Member

Can we get a rejected test for the following, please?:

@import url('a.css') tv; @import 'a.css' tv;

Disallow duplicate `@import` rules within a stylesheet.
```css

This comment has been minimized.

@jeddy3

jeddy3 Oct 15, 2017

Member

It's more common to see @imports on single lines, so let's have the example like that:

    @import "a.css";
    @import "a.css";
/** ↑
* These duplicates  */

agatac added some commits Oct 16, 2017

@agatac

This comment has been minimized.

Contributor

agatac commented Oct 16, 2017

I also added the following test (rejected):

@import url('a.css') tv, projection;
@import 'a.css' projection, screen, tv;

If I'm correct this should be rejected. The first import is a subset of the second one, therefore it's a duplicate. Let me know what you think.

@jeddy3

Oh wow, you've already made use of the media query parser. That's fantastic to see!

If I'm correct this should be rejected.

Yes, I believe that's correct.

I guess the last step is to account/test for non-boolean media features, e.g. I believe the following are duplicates:

@import "a.css" tv, (min-width : 500px);
@import url(a.css) (  min-width:500px   ), tv;

Do you think detecting these duplicates is achievable?

}
values.push(uri);
mediaQueries.push(...media);

This comment has been minimized.

@jeddy3

jeddy3 Oct 16, 2017

Member

Can you please workaround the lack of the spread operator in node@4? The tests are currently failing because of this.

@agatac

This comment has been minimized.

Contributor

agatac commented Oct 18, 2017

Do you think detecting these duplicates is achievable?

I used regex to remove spaces. The logic responsible for checking duplicates remains the same. Do you think it's correct?

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Oct 18, 2017

@agatac Thanks for your continued amends.

I used regex to remove spaces.

I was just thinking the same myself. Seems like a simple and eloquent solution.

The logic responsible for checking duplicates remains the same. Do you think it's correct?

Yes, I believe so.

@stylelint/core If someone has a spare moment, can you cast your eyes over this PR please? This rule will be added to config-recommended (and therefore config-standard), and so we should be confident it's robust.

@CAYdenberg

This comment has been minimized.

Contributor

CAYdenberg commented Oct 19, 2017

I don't have time to look in depth right now, but it looks to me like the mediaQueries is a flat array contain all mediaQueries that have been encountered? (I could be totally wrong about this - late night last night). So it seems like if you had:

@import "a.css" screen
@import "b.css" tv
@import "a.css" tv

it would fail inappropriately because both conditions for a duplicate have been met? I need to checkout the repo and play with it to see if that's the case, but in the mean time it wouldn't hurt to add something like that as a test?

@agatac

This comment has been minimized.

Contributor

agatac commented Oct 19, 2017

You're right! I added a passing test that you suggested and reworked the logic behind finding duplicates.

const isDuplicate =
imports.hasOwnProperty(uri) &&
(imports[uri].every(q => media.indexOf(q) !== -1) ||
media.every(q => imports[uri].indexOf(q) !== -1));

This comment has been minimized.

@CAYdenberg

CAYdenberg Oct 29, 2017

Contributor

I'm sure I'm missing something but can you just explain why it's necessary to check this in both directions? The idea is to see if ANY media have not been encountered before right? If prior @import running tally contains items that are NOT here, it would still be a duplicate, no?

Maybe you could add a comment clarifying this.

Also, since you're using imports[uri] to below to check whether this item exists, I think it would be clearer to do the same here instead of hasOwnProperty.

This comment has been minimized.

@agatac

agatac Oct 29, 2017

Contributor

If you asked me when I wrote that I'd probably give you a reason but now I'm not so convinced anymore. Do you think the following is better?

const isDuplicate =
  media.length
  ? imports[uri] && media.some(q => imports[uri].indexOf(q) !== -1)
  : imports[uri]

This comment has been minimized.

@CAYdenberg

CAYdenberg Oct 30, 2017

Contributor

If the tests pass yes I think that's much more clear. Thanks :)

This comment has been minimized.

@agatac

agatac Oct 30, 2017

Contributor

Done

@CAYdenberg

This comment has been minimized.

Contributor

CAYdenberg commented Oct 30, 2017

LTGM. You can probably remove the WIP as this is just waiting for approval from @Jeddy I believe.

const imports = {};
root.walkAtRules("import", atRule => {

This comment has been minimized.

@evilebottnawi

evilebottnawi Oct 30, 2017

Member

@import case insensitivity use /^import$/i, also need more tests around @IMPORT, @ImPoRt and etc

@agatac agatac changed the title from [WIP] add no-duplicate-at-import-rules first draft to Add no-duplicate-at-import-rules first draft Oct 31, 2017

@jwilsson

LGTM, with a small doc request.

/** ↑
* These are duplicates */
```

This comment has been minimized.

@jwilsson

jwilsson Oct 31, 2017

Member

Sorry for being so late on the ball, but could we get a line here saying something like "This rule ignores @import in Less.".

This is because postcss-less doesn't omit AtRule nodes, but Import nodes and we don't support that anyway.

@CAYdenberg

This comment has been minimized.

Contributor

CAYdenberg commented Nov 1, 2017

Out of curiosity, is "2 reviewers per PR" a rule, or more of a guideline? Four seems excessive in any case.

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Nov 1, 2017

Out of curiosity, is "2 reviewers per PR" a rule, or more of a guideline? Four seems excessive in any case.

It's a rule for ensuring quality. At least two reviewers (except small docs PRs). All reviewers should approve PR. CI should be green. Only then PR could be merged.

@jeddy3

jeddy3 approved these changes Nov 4, 2017 edited

Thanks everyone for continuing to review this.

LGTM.

@agatac Thanks for your patience... it's been a busy couple of weeks.

@jeddy3 jeddy3 merged commit eec8dd4 into stylelint:master Nov 4, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.01%) to 95.673%
Details
@jeddy3

This comment has been minimized.

Member

jeddy3 commented Nov 4, 2017

Changelog:

  • Added: no-duplicate-at-import-rules rule (#2963).

Sorry for being so late on the ball, but could we get a line here saying something like "This rule ignores @import in Less.".

Done in daf4d7f

@CAYdenberg

This comment has been minimized.

Contributor

CAYdenberg commented Nov 7, 2017

@agatac Thank you for your contribution!

@agatac

This comment has been minimized.

Contributor

agatac commented Nov 7, 2017

Thanks for being patient and helping me on the way :)

@s10wen

This comment has been minimized.

Contributor

s10wen commented Apr 23, 2018

Nice work all! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment