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 stylelint #1216

Closed
wants to merge 17 commits into from
Closed

Add stylelint #1216

wants to merge 17 commits into from

Conversation

karlhorky
Copy link

@karlhorky karlhorky commented Dec 9, 2016

Hi there! First of all, thanks to all the contributors for this awesome project. Having all the tools, configuration and setup in one place helps tons.

create-react-app doesn't have any CSS linting yet. I noticed that there was tentative agreement for further consideration in #636 (comment), so I decided to open a PR for this, since I need it for a project anyway.

I've included the standard config because I don't have a full opinionated stylelint config yet, but this can of course be changed.

Note: This depends on my fork of @vieron's stylelint webpack plugin, because the errors were not displaying correctly using watch mode in CRA. I've opened a pull request on the plugin here: webpack-contrib/stylelint-webpack-plugin#43

Edit: Pull request has been merged!

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@cr101
Copy link
Contributor

cr101 commented Dec 10, 2016

Or you could use a Sass linter for both SASS and SCSS syntax!

@karlhorky
Copy link
Author

karlhorky commented Dec 10, 2016

stylelint also supports CSS-like syntaxes like SCSS, SugarSS and Less. This is however not relevant for create-react-app, which currently only supports vanilla CSS.

The sasslint-webpack-plugin is also deprecated in favor of stylelint: https://github.com/alleyinteractive/sasslint-webpack-plugin/blob/master/README.md

@karlhorky karlhorky changed the title Add Stylelint Add stylelint Dec 14, 2016
@karlhorky
Copy link
Author

webpack-contrib/stylelint-webpack-plugin#43 has been merged and released as 0.4.1! I've updated the dependency accordingly.

@@ -60,6 +61,9 @@
"recursive-readdir": "2.1.0",
"strip-ansi": "3.0.1",
"style-loader": "0.13.1",
"stylelint": "7.6.0",
"stylelint-config-standard": "15.0.0",
"stylelint-webpack-plugin": "0.4.1",
Copy link

@JaKXz JaKXz Dec 18, 2016

Choose a reason for hiding this comment

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

@karlhorky I recommend another update here to capture our fixes from this morning (v0.4.2).

Copy link
Author

Choose a reason for hiding this comment

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

Done!

@gaearon
Copy link
Contributor

gaearon commented Feb 12, 2017

Can you provide some details about node_modules disk size before and after this change please?

Also, would this work well for people compiling Sass/Less into CSS, and importing the compiled CSS?

Does this plugin work well with Webpack 2?

@karlhorky
Copy link
Author

karlhorky commented Feb 12, 2017

some details about node_modules disk size before and after

du -sh node_modules before and after (13M difference):

109M    node_modules # before
122M    node_modules # after

would this work well for people compiling Sass/Less into CSS

It could lint the original source Sass/Less if I changed the file extensions that are matched by the plugin. I however only enable linting of source files (disabling the linting of any built CSS files) because the built output will be optimized and is thus not a good candidate for linting.

Does this plugin work well with Webpack 2?

I tested this with a simple application and the plugin works. As for working well, I haven't tested extensively yet - maybe @JaKXz can offer some insight here?

@karlhorky
Copy link
Author

karlhorky commented Feb 12, 2017

I've modified the eject script to create a stylelint configuration entry as well, although webpack-contrib/stylelint-webpack-plugin#67 may interfere until it's resolved.

Edit: This appears to be resolved in 0.6.0. Bumping the version.

@JaKXz
Copy link

JaKXz commented Feb 12, 2017

Does this plugin work well with Webpack 2?

The plugin is actively tested against webpack 2 as of webpack-contrib/stylelint-webpack-plugin#46, so I have confidence in the logic against webpack's compiler [from v0.5.0 on].

@gaearon
Copy link
Contributor

gaearon commented Feb 13, 2017

It could lint the original source Sass/Less if I changed the file extensions that are matched by the plugin. I however only enable linting of source files (disabling the linting of any built CSS files) because the built output will be optimized and is thus not a good candidate for linting.

The scenario I meant is when people compile Sass to CSS but keep it in src next to the components. That seemed like the most straightforward way to use a preprocessor to me.

@karlhorky
Copy link
Author

compile Sass to CSS but keep it in src next to the components

Ah yeah, that would also be supported by changing the file extensions.

configFile: path.join(__dirname, '../.stylelintrc'),
// @remove-on-eject-end
context: paths.appSrc,
files: ['**/*.css'],
Copy link

Choose a reason for hiding this comment

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

one thing to consider is that stylelint can handle .sass/.scss files, and the default glob for stylelint-webpack-plugin supports both so you could drop this line.

Copy link
Author

Choose a reason for hiding this comment

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

Ah, nice idea. If @gaearon agrees we want to go this direction, I will make this change.

@karlhorky
Copy link
Author

@gaearon any feedback to the files config option? Should I just remove it as @JaKXz suggested to allow Stylelint to lint all supported file extensions?

@shai32
Copy link

shai32 commented Apr 14, 2017

Is this PR still active?, will it be merged?

@karlhorky
Copy link
Author

If there's any interest I can rebase and fix conflicts (and upgrade to webpack 2 if necessary).

@roytz
Copy link

roytz commented Apr 14, 2017

There is much interest :) (and to the upgrade to webpack 2 too)

@cr101
Copy link
Contributor

cr101 commented Apr 15, 2017

Handling .sass/.scss files is essential

@shai32
Copy link

shai32 commented Apr 25, 2017

@karlhorky please upgrade to webpack 2

@shai32
Copy link

shai32 commented May 2, 2017

Is this pr active?

@karlhorky
Copy link
Author

Ok, I'll get to this in the next day or so.

@gaearon gaearon changed the base branch from master to next January 12, 2018 20:09
@gaearon
Copy link
Contributor

gaearon commented Jan 12, 2018

I'd also like to better understand the performance implications of this. If the project has e.g. 500 300-line CSS files, how much does this plugin affect the development and production build times?

People already complain about the builds being super slow so I'm not really sure we have the capacity to add any more build-time work.

@JaKXz
Copy link

JaKXz commented Jan 16, 2018

Perhaps this should be opt-in like some others in #3815? I think the lintDirtyModulesOnly option will help wrt build-time work, so if you have 500 files but only a few have changed then only those will be linted. The separate lint task (#2729) can just call stylelint directly for all files.

FYI v0.10.1 of the plugin was released recently as well as stylelint v8.

@gaearon
Copy link
Contributor

gaearon commented Jan 16, 2018

Perhaps this should be opt-in like some others in #3815?

This seems a bit trickier. We won't make those opt-in by some configuration flag. Instead if you import .sass, we'll ask you to install node-sass. If you install it, Sass will "just work". Similarly, if you import .module.css instead of .css, we will treat it as CSS Modules. But with Stylelint we can't "guess" whether you want it or not. We also don't know if you're writing CSS or compiling to it. I'm also not sure if it can handle CSS Modules. So it seems like this is potentially more involved than it seems. Could the user just install and run Stylelint themselves? I'm not sure it needs to be integrated with the system in the same way that e.g. Sass needs to be for a good experience.

@JaKXz
Copy link

JaKXz commented Jan 17, 2018

I think the intention here is to have stylelint errors side by side with eslint errors in the build, and assuming CRA works with the better errors webpack plugin that should be achievable :) but I understand your point, I agree it's much harder to pinpoint exactly what the user would want from the outset, and that could change with a lot more volatility.

@gaearon gaearon modified the milestones: 2.0.0, 2.x Jan 20, 2018
@gaearon
Copy link
Contributor

gaearon commented Jan 20, 2018

Retagging as 2.x because I don't think this should block the 2.0 release.
If we can get the numbers that show it's not expensive, sure, let's get it in.

If it slows down our already slow builds I don't think we can do this (but you can always use it separately in your IDE etc).

@caghand
Copy link

caghand commented Jan 22, 2018

Yes, you can use stylelint in your IDE, but since the errors do not appear in the build output (like eslint), you won't be able to implement a check-in policy that prevents style-violating CSS from being pushed in. :)

-Caghan

@sstruct
Copy link
Contributor

sstruct commented Feb 9, 2018

I'd prefer Prettier

rules: {
'at-rule-no-unknown': true,
'color-no-invalid-hex': true,
'declaration-block-no-duplicate-properties': true,

Choose a reason for hiding this comment

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

Wouldn't be better to use declaration-block-no-duplicate-properties with ignore: ["consecutive-duplicates"]?

Moreover, is this rule blocks the pattern of (Added example)?

margin: 1rem;
margin-bottom: 2rem;

@Timer Timer closed this Sep 26, 2018
@Timer Timer reopened this Sep 26, 2018
@ai
Copy link
Contributor

ai commented Sep 26, 2018

If we have ESLint for JS, we definitely should have Stylelint for CSS.

@gaearon gaearon closed this Nov 1, 2018
@lock lock bot locked and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.