Add stylelint #11767

simison · 2019-03-29T16:22:25Z

Adds linting for SCSS files, using WordPress core ruleset.

TODO in follow up PRs:

Add this linter to pre-commit-hook
Actually fix the errors. yarn reformat-files handles a lot of them,
Fail on CI lint task on errors

Changes proposed in this Pull Request:

Adds Stylelint setup and a couple package.json scripts to use it.

Testing instructions:

yarn lint:scss / yarn lint
yarn reformat-files:scss / yarn reformat-files

Proposed changelog entry for your changes:

ockham · 2019-03-29T16:30:23Z

Hmm, yarn lint:css doesn't complain about the undefined var in jetpack-plugin-sidebar.scss though (which was kinda the point of this, wasn't it? (see pMz3w-9H7-p2#comment-69629)

Also, let's ideally include Calypso's .stylelintrc in the @automattic/calypso-build package instead of copy-pasting it here 😬

jetpackbot · 2019-03-29T16:31:58Z

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: April 2, 2019.
Scheduled code freeze: March 26, 2019

Generated by 🚫 dangerJS against f6eaeb5

simison · 2019-03-29T16:35:31Z

Also, let's ideally include Calypso's .stylelintrc in the @automattic/calypso-build package instead of copy-pasting it here 😬

I was thinking it should be a separate package just like for eslint?

Here I copypasted it just for the proof of concept.

matticbot · 2019-04-18T11:42:02Z

Caution: This PR has changes that must be merged to WordPress.com
Hello simison! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D27105-code before merging this PR. Thank you!

jetpackbot · 2019-04-18T11:44:16Z

	Warnings
⚠️	The Privacy section is missing for this PR. Please specify whether this PR includes any changes to data or privacy.

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 523a342

simison · 2019-04-18T18:39:44Z

@Automattic/jetpack-crew a question: which ruleset should Jetpack follow for linting scss files — stylelint-config-wordpress or the one from Calypso?

You can see the difference roughly by first running yarn reformat-files:css with current setup, and then do the same after you copy Calypso stylelint file to .stylelintrc.json.

I didn't dig deep yet to differences but brackets spacing seems different at least:

darken($alert-green, 14%); WordPress
darken( $alert-green, 14% ); Calypso

There's no big difference in maintenance from Jetpack point of view — both would be external configs Jetpack would just consume and use. I think it's also possible to layer them by using WordPress as a base and Calypso on top of it.

package.json

matticbot · 2019-10-08T16:52:37Z

Caution: This PR has changes that must be merged to WordPress.com
Hello simison! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D33725-code before merging this PR. Thank you!

simison · 2019-10-08T17:04:36Z

@jeherve @kraftbj do you have an opinion which Stylelint config to use for Jetpack? I would suggest sticking to the core one.

See #11767 (comment)

matticbot · 2019-10-08T17:09:09Z

simison, Your synced wpcom patch D33725-code has been updated.

matticbot · 2019-10-08T18:37:39Z

simison, Your synced wpcom patch D33725-code has been updated.

jeherve

Sorry, somehow I missed this PR before. I like it!

I would suggest sticking to the core one.

I would vote for sticking to the Core one too.

package.json

jeherve · 2019-10-09T11:13:37Z

package.json

@@ -192,13 +196,16 @@
 		"mockery": "2.1.0",
 		"nock": "10.0.6",
 		"node-wp-i18n": "1.2.3",
+		"npm-run-all": "4.1.5",


Do we need to introduce a new dependency here, instead of just chaining the different actions?

We can totally just chain them but it should be faster to run in CI in parallel. How much faster — I'd have to check. :-)

I wonder if it would make sense to add php:lint script to the same run? (That would be good for another PR anyway)

Yeah, if it is faster we should probably start using it everywhere we can :)
But we can do that in another PR indeed.

Yeah, I'd like to see some exploration adding parallelism to build steps for instance.

Noting that we started using concurrently for some of our tasks; maybe we could use it here too?

Oh ye, totes agreed! Feel free to change unless I get to it first.

@jeherve switched to concurrently

matticbot · 2019-10-10T08:49:48Z

simison, Your synced wpcom patch D33725-code has been updated.

jeherve

This works for me. Tests are failing right now though:
https://travis-ci.org/Automattic/jetpack/jobs/596002607

simison · 2019-10-10T10:31:26Z

Yeah, that's a weird one. I don't understand why and I was suspecting it was because of how React version got resolved from [email protected] (hence #13705) but I'm not sure anymore.

Full error:

ERROR in Invariant Violation: Invalid hook call. Hooks can only be called inside of the body of a function component. This could happen for one of the following reasons:
    1. You might have mismatching versions of React and the renderer (such as React DOM)
    2. You might be breaking the Rules of Hooks
    3. You might have more than one copy of React in the same app
    See https://fb.me/react-invalid-hook-call for tips about how to debug and fix this problem.
        at invariant (webpack:///./node_modules/@wordpress/data/node_modules/react/cjs/react.development.js?:88:15)
        at resolveDispatcher (webpack:///./node_modules/@wordpress/data/node_modules/react/cjs/react.development.js?:1436:28)
        at useContext (webpack:///./node_modules/@wordpress/data/node_modules/react/cjs/react.development.js?:1441:20)
        at useRegistry (webpack:///./node_modules/@wordpress/data/build-module/components/registry-provider/use-registry.js?:55:79)
        at useDispatchWithMap (webpack:///./node_modules/@wordpress/data/build-module/components/use-dispatch/use-dispatch-with-map.js?:50:97)
        at eval (webpack:///./node_modules/@wordpress/data/build-module/components/with-dispatch/index.js?:105:99)
        at processChild (webpack:///./node_modules/react-dom/cjs/react-dom-server.browser.development.js?:2887:14)
        at resolve (webpack:///./node_modules/react-dom/cjs/react-dom-server.browser.development.js?:2811:5)
        at ReactDOMServerRenderer.render (webpack:///./node_modules/react-dom/cjs/react-dom-server.browser.development.js?:3201:22)
        at ReactDOMServerRenderer.read (webpack:///./node_modules/react-dom/cjs/react-dom-server.browser.development.js?:3160:29)
    Child mini-css-extract-plugin node_modules/css-loader/dist/cjs.js??ref--5-1!node_modules/postcss-loader/src/index.js??ref--5-2!node_modules/@automattic/calypso-build/node_modules/sass-loader/lib/loader.js??ref--5-3!extensions/shared/components/block-nudge/style.scss:
        Entrypoint mini-css-extract-plugin = *
           2 modules
    Child mini-css-extract-plugin node_modules/css-loader/dist/cjs.js??ref--5-1!node_modules/postcss-loader/src/index.js??ref--5-2!node_modules/@automattic/calypso-build/node_modules/sass-loader/lib/loader.js??ref--5-3!extensions/shared/components/style.scss:
        Entrypoint mini-css-extract-plugin = *
           2 modules
    Child mini-css-extract-plugin node_modules/css-loader/dist/cjs.js??ref--5-1!node_modules/postcss-loader/src/index.js??ref--5-2!node_modules/@automattic/calypso-build/node_modules/sass-loader/lib/loader.js??ref--5-3!extensions/shared/components/upgrade-nudge/style.scss:
        Entrypoint mini-css-extract-plugin = *
           2 modules

kraftbj · 2019-10-12T05:15:21Z

Late to the party, but agreed with sticking with Core.

stale · 2020-08-24T12:07:35Z

This PR has been marked as stale. This happened because:

It has been inactive in the past 3 months.
It hasn’t been labeled `[Pri] Blocker`, `[Pri] High`.

No further action is needed. But it's worth checking if this PR has clear testing instructions, is it up to date with master, and it is still valid. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.

simison · 2021-01-15T10:19:19Z

@jeherve looks like this didn't get merged last time it was ready — I rebased it to new folder structure and switched to new core package.

Ready to go on my behalf! :-)

Works just fine without but wanted to keep all `concurrently` calls more consistent

simison · 2021-01-27T10:22:16Z

@jeherve rebased. I've switched to concurrently earlier: #11767 (comment)

Let's get this long timer in? :-)

anomiex

Seems ok as far as it goes. A few nitpicks inline.

But if we're going to add this, we should fully add it. There's not a whole lot of point in adding linting if no one pays any attention to it.

The pre-commit hook (tools/git-hooks/pre-commit-hook.js) should probably run it like it does phpcs and eslint.
CI should run it too.
And both of the above would likely want a tool like our eslint-changed and logic like our excludelists so as to not bomb everyone with 10281 warnings all the time. Or else someone needs to fix the 10281 warnings it currently reports 😉.

Is that on anyone's radar?

anomiex · 2021-01-27T19:53:07Z

package.json

-		"reformat-files": "yarn prettier --ignore-path .eslintignore --write \"**/*.{js,jsx,json}\"",
+		"reformat-files": "yarn concurrently 'yarn:reformat-files:*'",
+		"reformat-files:js": "yarn prettier --ignore-path .eslintignore --write \"**/*.{js,jsx,json}\"",
+		"reformat-files:scss": "yarn lint:scss --fix",


It sounds like a --fix is going to do more than just reformat...

Neither Prettier nor Stylelint changes actual functionality so I think it's exactly reformatting, but I'm no native speaker. :-)

The command was to follow existing convention from Calypso repo.

Not a biggie to use something else here, tho!

anomiex · 2021-01-27T19:57:07Z

package.json

@@ -59,14 +59,18 @@
 		"docker:update-core-unit-tests": "yarn docker:compose exec wordpress svn up /tmp/wordpress-develop/tests/phpunit/data/ /tmp/wordpress-develop/tests/phpunit/includes",
 		"docker:wp": "yarn docker:compose exec wordpress wp --allow-root --path=/var/www/html/",
 		"install-if-deps-outdated": "yarn install --check-files --production=false --frozen-lockfile",
-		"lint": "yarn lint-file .",
+		"lint": "yarn concurrently 'yarn:lint:*'",


The resulting output isn't very human-friendly. In a few places it even has lines from the different linters interleaved.

simison · 2022-12-19T09:37:10Z

This is pretty old and Jetpack structure is way different these days. I'll close and anyone feel free to re-open/pick code and continue.

simison added [Status] In Progress DO NOT MERGE don't merge it! labels Mar 29, 2019

simison requested a review from a team March 29, 2019 16:22

simison force-pushed the add/stylelint branch from ecbbdb0 to f6eaeb5 Compare April 1, 2019 09:40

simison mentioned this pull request Apr 1, 2019

Package calypso stylelint config Automattic/wp-calypso#31916

Closed

simison force-pushed the add/stylelint branch from f6eaeb5 to 7a03850 Compare April 18, 2019 11:41

matticbot added the Touches WP.com Files label Apr 18, 2019

simison added the Build label Apr 18, 2019

simison commented Apr 19, 2019

View reviewed changes

package.json Outdated Show resolved Hide resolved

simison force-pushed the add/stylelint branch from 7a03850 to 3154984 Compare October 8, 2019 16:52

simison added [Status] Needs Review This PR is ready for review. and removed DO NOT MERGE don't merge it! [Status] In Progress labels Oct 8, 2019

simison added this to the 7.9 milestone Oct 8, 2019

jeherve reviewed Oct 9, 2019

View reviewed changes

jeherve assigned simison Oct 9, 2019

jeherve reviewed Oct 10, 2019

View reviewed changes

stale bot added the [Status] Stale label Aug 24, 2020

simison force-pushed the add/stylelint branch from fd93724 to 69a7ba8 Compare January 15, 2021 10:07

stale bot removed the [Status] Stale label Jan 15, 2021

simison added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Jan 15, 2021

matticbot removed the Touches WP.com Files label Jan 16, 2021

simison force-pushed the add/stylelint branch from 0e416a1 to af7dd17 Compare January 16, 2021 12:39

simison added 5 commits January 27, 2021 11:59

Add stylelint

c672979

Add back reformat-files

ee7bc43

Fix lint-required script

4c6eb29

Switch to concurrently

2c381fb

Prefix with yarn for consistency

523a342

Works just fine without but wanted to keep all `concurrently` calls more consistent

simison force-pushed the add/stylelint branch from af7dd17 to 523a342 Compare January 27, 2021 10:03

simison added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Jan 27, 2021

anomiex reviewed Jan 27, 2021

View reviewed changes

kraftbj added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Jan 27, 2021

kraftbj unassigned simison Mar 23, 2021

jeherve mentioned this pull request Feb 22, 2022

My Jetpack: improve My Jetpack link styles #23007

Merged

simison closed this Dec 19, 2022

simison deleted the add/stylelint branch December 19, 2022 09:37

github-actions bot removed the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Dec 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add stylelint #11767

Add stylelint #11767

simison commented Mar 29, 2019 •

edited

Loading

ockham commented Mar 29, 2019 •

edited by simison

Loading

jetpackbot commented Mar 29, 2019 •

edited

Loading

simison commented Mar 29, 2019

matticbot commented Apr 18, 2019

jetpackbot commented Apr 18, 2019 •

edited

Loading

simison commented Apr 18, 2019 •

edited

Loading

matticbot commented Oct 8, 2019

simison commented Oct 8, 2019 •

edited

Loading

matticbot commented Oct 8, 2019

matticbot commented Oct 8, 2019

jeherve left a comment

jeherve Oct 9, 2019

simison Oct 9, 2019

jeherve Oct 9, 2019 •

edited

Loading

simison Oct 9, 2019

jeherve Jan 15, 2021

simison Jan 15, 2021 •

edited

Loading

simison Jan 16, 2021

matticbot commented Oct 10, 2019

jeherve left a comment

simison commented Oct 10, 2019 •

edited

Loading

kraftbj commented Oct 12, 2019

stale bot commented Aug 24, 2020

simison commented Jan 15, 2021

simison commented Jan 27, 2021

anomiex left a comment

anomiex Jan 27, 2021

simison Dec 19, 2022

anomiex Jan 27, 2021

simison commented Dec 19, 2022

Add stylelint #11767

Add stylelint #11767

Conversation

simison commented Mar 29, 2019 • edited Loading

TODO in follow up PRs:

Changes proposed in this Pull Request:

Testing instructions:

Proposed changelog entry for your changes:

ockham commented Mar 29, 2019 • edited by simison Loading

jetpackbot commented Mar 29, 2019 • edited Loading

simison commented Mar 29, 2019

matticbot commented Apr 18, 2019

jetpackbot commented Apr 18, 2019 • edited Loading

simison commented Apr 18, 2019 • edited Loading

matticbot commented Oct 8, 2019

simison commented Oct 8, 2019 • edited Loading

matticbot commented Oct 8, 2019

matticbot commented Oct 8, 2019

jeherve left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeherve Oct 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simison Jan 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matticbot commented Oct 10, 2019

jeherve left a comment

Choose a reason for hiding this comment

simison commented Oct 10, 2019 • edited Loading

kraftbj commented Oct 12, 2019

stale bot commented Aug 24, 2020

simison commented Jan 15, 2021

simison commented Jan 27, 2021

anomiex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simison commented Dec 19, 2022

simison commented Mar 29, 2019 •

edited

Loading

ockham commented Mar 29, 2019 •

edited by simison

Loading

jetpackbot commented Mar 29, 2019 •

edited

Loading

jetpackbot commented Apr 18, 2019 •

edited

Loading

simison commented Apr 18, 2019 •

edited

Loading

simison commented Oct 8, 2019 •

edited

Loading

jeherve Oct 9, 2019 •

edited

Loading

simison Jan 15, 2021 •

edited

Loading

simison commented Oct 10, 2019 •

edited

Loading