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

Update stylelint and the official WP package #275

Merged
merged 3 commits into from
Sep 16, 2022

Conversation

tomjn
Copy link
Contributor

@tomjn tomjn commented Feb 17, 2022

This contribute towards #274 by upgrading the official WP package used for configuration and upgrading stylelint to 14.2 which was required by the WP package update. This fixes some deprecation notices that show up downstream in projects

packages/stylelint-config/package-lock.json Outdated Show resolved Hide resolved
packages/stylelint-config/package.json Outdated Show resolved Hide resolved
@tomjn
Copy link
Contributor Author

tomjn commented Feb 18, 2022

@tfrommen there are changes necessary in addition to these, specifically handling of SASS/SCSS changed in Stylelint 14, but I forgot to mark it as a draft and can't find how to change it back

@tomjn tomjn marked this pull request as draft February 18, 2022 15:27
@tomjn
Copy link
Contributor Author

tomjn commented Feb 18, 2022

found it, it's in the reviewers section of the sidebar

@mattheu
Copy link
Member

mattheu commented Jul 28, 2022

there are changes necessary in addition to these, specifically handling of SASS/SCSS changed in Stylelint 14, but I forgot to mark it as a draft and can't find how to change it back

I'd love to push this PR forwards. What are these changes? Stylelint docs say that if you're wanting to lint scss files you need the rules in a separate package. But we're actually extending the WordPress stylelint config, which already handles this. So all is fine I think.

@tomjn
Copy link
Contributor Author

tomjn commented Jul 28, 2022

I'd love to push this PR forwards. What are these changes? Stylelint docs say that if you're wanting to lint scss files you need the rules in a separate package. But we're actually extending the WordPress stylelint config, which already handles this. So all is fine I think.

@mattheu if you checkout the branch you'll see there's more to it than just updating the package, configs need accounting for as well as several unknowns I never reached because of that. In moving to a newer major version Stylelint made config changes as well as rule changes that I hadn't anticipated and didn't have time to investigate. If core have resolved those and we can upgrade by upgrading a dependency in WP then that's good but I'm unsure what additional changes are necessary here

@mattheu
Copy link
Member

mattheu commented Jul 29, 2022

I know this was a while back so understand that you don't remember the details of what problems you hit, so i'll try and reproduce to see.

I checked out this branch and used it to lint the SCSS from a large project currently using @humanmade/stylelint-config v1.1.1. I'll summarise the issues below.

  • The custom config on that project used the rules declaration-property-unit-blacklist and declaration-property-unit-whitelist which are now removed and should be replaced with declaration-property-unit-disallowed-list and declaration-property-unit-allowed-list.
  • Some new rules now causing it to fail
    • No longer allowed to use scss global functions. e.g. switch from str-index to string.index
    • No longer allowed to use @extend with classes e.g. @extend .my-component and should prefer using extend placeholders.
    • No longer alllowed to use the preceeding underscore when importing a partial. e.g. @import "_partial should now be @import "partial" as sass handles this OK. Similarly no need to specify the extension so @import partial.scss` is also now flagged.

In my opinion, these changes are all fine and to be expected when updating to a new major release of the linting rules. We should ensure this is rolled out in a major release of our rules too, and document that there are some changes, but otherwise I see no problem with this.

@tomjn tomjn marked this pull request as ready for review September 1, 2022 13:35
@ntwb
Copy link
Member

ntwb commented Sep 16, 2022

Via https://github.com/WordPress/gutenberg/blob/trunk/packages/stylelint-config/CHANGELOG.md

## 21.0.0 (2022-08-24)

### Breaking Change

    Increase the minimum Node.js version to 14 (https://github.com/WordPress/gutenberg/pull/43141).

## 20.0.0 (2022-01-27)

### Breaking Change

-   Increased minimum peer dependency of `stylelint` to `14.2.0` ([#38091](https://github.com/WordPress/gutenberg/pull/38091)). See [official migration guide to v14](https://github.com/stylelint/stylelint/blob/14.0.0/docs/migration-guide/to-14.md) for details.

## 19.0.0 (2021-01-21)

### Breaking Change

-   Increase the minimum Node.js version to 12 ([#27934](https://github.com/WordPress/gutenberg/pull/27934)).
-   Increased minimum peer dependency of `stylelint` to `13.7.0`.

### Bug Fixes

-   Fixed deprecation warning for `declaration-property-unit-whitelist`.

Copy link
Member

@ntwb ntwb left a comment

Choose a reason for hiding this comment

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

Thanks everyone, apologies it took a little while to get this over the line

@ntwb ntwb merged commit ba2cd1f into master Sep 16, 2022
@ntwb ntwb deleted the stylelint-package-update-v20 branch September 16, 2022 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants