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

[dev] Replace sass-lint with stylelint #86177

Merged
merged 41 commits into from
Jan 15, 2021
Merged

[dev] Replace sass-lint with stylelint #86177

merged 41 commits into from
Jan 15, 2021

Conversation

jbudz
Copy link
Member

@jbudz jbudz commented Dec 16, 2020

sass-lint is unmaintained, so this replaces it with
an actively maintained linter. The changeset here includes two entry
points

  1. node scripts/stylelint
  2. via node scripts/register_precommit_hook on staged files

Closes #79323

sass-lint is unmaintained, so this replaces it with
an actively maintained linter.  The changeset here includes two entry
points

1) node scripts/stylelint **/*.scss for general linting
2) via node scripts/register_precommit_hook on staged filed

Lint rules and approach to migrating is TBD.  I'm opening this now to
showcase the code changes separately from a large lint changeset, and
will circle back before merging depending on our approach.
@jbudz jbudz added Team:Operations Team label for Operations Team v8.0.0 v7.12.0 labels Dec 16, 2020
.stylelintrc Outdated Show resolved Hide resolved
@jbudz jbudz marked this pull request as ready for review December 18, 2020 19:38
@jbudz jbudz requested review from a team as code owners December 18, 2020 19:38
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-operations (Team:Operations)

@jbudz jbudz added the release_note:skip Skip the PR/issue when compiling release notes label Dec 18, 2020
@@ -12,12 +12,12 @@ object Lint : BuildType({

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: also update the task description to replace sasslint with stylelint.

Copy link
Member Author

Choose a reason for hiding this comment

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

pushed with b4d9ab8

@botelastic botelastic bot added the Feature:Embedding Embedding content via iFrame label Dec 21, 2020
package.json Outdated Show resolved Hide resolved
@@ -18,4 +18,4 @@
*/

require('../src/setup_node_env');
require('../src/dev/run_sasslint');
require('../src/dev/run_stylelint');
Copy link
Contributor

Choose a reason for hiding this comment

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

Running this script directly outputs the stylelint help.

Copy link
Member Author

Choose a reason for hiding this comment

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

Pushed d3cedd7. It uses the stylelint CLI parser to check for input globs, and if there aren't any we default to *.s(c|a)ss.

@jbudz
Copy link
Member Author

jbudz commented Jan 12, 2021

@elasticmachine merge upstream

@jbudz
Copy link
Member Author

jbudz commented Jan 12, 2021

@snide I brought in most of the error rules from our sass-lint configurations. There were some missing analogues, e.g. border:none. Eyes from design would be helpful, we're targeting 7.12, so ~1 month if that's possible. I can push any review changes, or commits directly to this branch are fine.

@snide
Copy link
Contributor

snide commented Jan 12, 2021

@jbudz I'll check this out later today. Thanks for doing the setup!

@snide
Copy link
Contributor

snide commented Jan 13, 2021

Just an update here. This system works perefectly fine and I don't expect any changes on the engineering side. I'm adjusting some rules to be more strict and fixing sass as they fail. I should have a commit for this later today with a ✔️ .

@snide
Copy link
Contributor

snide commented Jan 14, 2021

@jbudz PR for you. I added a couple dozen more rules and kicked the tires. Works great, and love that it can autofix. There's some more I'd like to add to this, but it can wait for a later PR when I have more time.

jbudz#2

Issues

At least in VIM, I noticed that the prettier autosave settings were conflicting with the style-lint settings. I'd like to have @cchaos or someone check to see what happens in VSCode. I see there are some other extensions we can run for prettier similar to what we're doing with ESLint, but after messing with them for a bit, I wasn't smart enough to get them running. I want make sure this isn't just something my VIM extensions are doing wrong with prettier first, but that may cause problems for this PR if we're seeing it in other editors.

@jbudz
Copy link
Member Author

jbudz commented Jan 14, 2021

@snide I was able to reproduce the prettier/stylelint conflict. The most straightforward option is to disable prettier on .scss files (pushed with a951ded) and rely on stylelint for style and errors. We haven't had any enforcement on prettier formatting to date, so it's hit or miss if our files are formatted properly.

I looked at plugins - https://github.com/hugomrdias/prettier-stylelint looks like what we would want but it's not an active project.

The third option is to align our style changes with prettier's default. I'm not seeing a way to do the reverse, e.g. prettier/prettier#2705. This would involve migrating towards https://github.com/stylelint/stylelint-config-standard/ and a closer look at the prettier defaults.

Any thoughts?

@snide
Copy link
Contributor

snide commented Jan 14, 2021

@jbudz I think that's a good option for now. People can use extensions in their editors for the style-lint warnings. Mostly wanted to make sure we didn't run into frustrations with people not being able to save files that style-lint liked.

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

This looks OK from design. We'll add to the rules over time in separate PRs. Thanks for setting this up.

@jbudz
Copy link
Member Author

jbudz commented Jan 15, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
advancedSettings 919.6KB 919.6KB -24.0B
canvas 1.6MB 1.6MB +216.0B
discover 511.3KB 511.3KB -12.0B
enterpriseSearch 1.8MB 1.8MB -67.0B
graph 1.3MB 1.3MB -28.0B
home 215.4KB 215.3KB -132.0B
indexManagement 1.5MB 1.5MB -4.0B
kibanaLegacy 146.9KB 146.9KB -4.0B
kibanaReact 350.2KB 349.9KB -320.0B
lens 1.1MB 1.1MB +108.0B
ml 7.1MB 7.1MB -1.3KB
visTypeTable 153.7KB 153.7KB -12.0B
visTypeVislib 643.7KB 643.6KB -76.0B
total -1.7KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
data 1003.8KB 1003.8KB -8.0B
embeddable 233.2KB 233.3KB +104.0B
indexManagement 129.1KB 129.1KB +4.0B
maps 154.4KB 154.4KB -16.0B
mapsLegacy 89.4KB 89.4KB -4.0B
transform 24.5KB 24.5KB -8.0B
upgradeAssistant 60.1KB 60.1KB +29.0B
visTypeTimeseries 137.4KB 137.4KB -8.0B
visualizations 170.3KB 170.1KB -160.0B
total -67.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@jbudz jbudz merged commit 51ba94d into elastic:master Jan 15, 2021
@jbudz jbudz deleted the stylelint branch January 15, 2021 17:52
jbudz added a commit to jbudz/kibana that referenced this pull request Jan 15, 2021
Co-authored-by: Tyler Smalley <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Dave Snider <[email protected]>
jbudz added a commit that referenced this pull request Jan 15, 2021
Co-authored-by: Tyler Smalley <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Dave Snider <[email protected]>

Co-authored-by: Tyler Smalley <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Dave Snider <[email protected]>
@jbudz
Copy link
Member Author

jbudz commented Jan 15, 2021

7.x: 1dc3276

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported Feature:Embedding Embedding content via iFrame release_note:skip Skip the PR/issue when compiling release notes Team:Operations Team label for Operations Team v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove or replace sass-lint dependency
6 participants