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

Make @import rules require a semicolon #6

Open
trainiac opened this issue Feb 15, 2017 · 7 comments
Open

Make @import rules require a semicolon #6

trainiac opened this issue Feb 15, 2017 · 7 comments

Comments

@trainiac
Copy link

I created an issue in the stylelint repo, but they believed this repository would be a better candidate for this type of feature.

The problem is, if you forget to include a semicolon the import on the following line will not get exectued which is pretty annoying not to have a warning for when developing.

@trainiac
Copy link
Author

@lahmatiy Also, thanks for 1.1.0 release. Both Sass and CSS Modules syntax work well in my repository.

@lahmatiy
Copy link
Member

PostCSS's AST doesn't store information about semicolons. So @import 'foo' and @import 'foo'; looks the same. No clue how to detect a problem. Moreover when you omit a semicolon it includes following until next semicolon or curly bracket to at-rule expression, i.e.

@import url('styles/elements')
@import url('styles/ui/base');

represents as single AST node:

{
  "type": "atrule",
  "name": "import",
  "params": "url('styles/elements')\n@import url('styles/ui/base')",
  ..
}

It's a quite complicated issue. Actually there should be a parse error.

Currently CSSTree lexer (that using by this plugin) tests declaration values only. It's going to support validation for at-rules and selectors a little bit later. Then it will detect that something wrong is going after url. But we need parse an at-rule expression before validation and parsing will fail (on @import inside at-rule expression).

Summarise, the best we can get is warning about parsing on second @import. It can help to figure out that's something wrong nearby. But this is not exactly what you'd expect.

@trainiac
Copy link
Author

Makes sense. Thanks for the explanation.

As mentioned in the other issue, I do get an indentation error warning from stylelint that at least lets me know something is wrong with the imports, but during development I only have errors enabled that lead to severe errors in the application being able to perform. Indentation errors only frustrate me while I'm developing, so I leave it as a warning until committing code. However leaving out a semicolon will fail to include the next line import and results in what I would say a pretty severe error that I would want to know about right away.

@trainiac
Copy link
Author

looking forward to the updates ;)

@lemori
Copy link

lemori commented Sep 19, 2017

Hi, I found that semicolon really matters.

Sample file:

@import './base.css'

.abc, .def {
  position: absolute;
}

.abc {
  display: flex;
}

When compiled by postcss & css-loader, .abc, .def { ... } is just gone, without any warnings or errors.
The same for stylelint (tried stylelint-webpack-plugin).

It took me a long time debugging (and founded in css-loader-parser of css-loader/lib/processCss.js).

It might be a postcss AST problem, but hopefully if there will be a parse error :)

@lahmatiy
Copy link
Member

CSSTree is not ready for a full validation checking of at-rules. Anyway it's going that direction and very close to it.
The main problem with stylelint & postcss that non-CSS syntax may to be used as input. In this case any CSS syntax extensions used in a at-rule prelude would lead to unwanted error. I think about a workaround. Although maybe I do not need to worry about it ;)

@lemori
Copy link

lemori commented Sep 20, 2017

Oh, what's the workaround?
Maybe we just have to use it with caution. Anyway, it's easy to identify.

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

No branches or pull requests

3 participants