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 to Stylelint 15 #32

Merged
merged 3 commits into from
Jul 27, 2023
Merged

Update to Stylelint 15 #32

merged 3 commits into from
Jul 27, 2023

Conversation

kevindew
Copy link
Member

@kevindew kevindew commented Apr 25, 2023

This PR pulls in a few contributions from #30, #36 and #44 to update this project to use Stylelint 15.

The upgrade to Stylelint 15 includes:

  • Checks for media query syntax, rules and values
  • Checks for number precision inside CSS functions
  • Fixes for duplicate rules with intentionally different units

But includes one major change worth shouting about:

Breaking change: Removal of stylistic rules

For more information, our rules configuration was updated in a separate PR:

We have removed all stylistic rules (such as tabs/spaces, indentation, etc) as they're no longer included in Stylelint common configurations as part of their deprecation in Stylelint 15.

Read about other breaking changes in the Stylelint Migrating to v15.0.0 guide.

@colinrotherham
Copy link
Contributor

Hi @kevindew I've rebased this PR as stylelint@15.7.0 came out last week

I'll see if I can fit in some testing on govuk-frontend

@kevindew
Copy link
Member Author

Great stuff @colinrotherham - do you happen to know if any views are forming about the use of "Prettier".

I'm somewhat expecting that the release path for this will be us saying "these type of rules won't be checked any more, if you care about them use prettier, if you don't accept that part of your SCSS won't be linted anymore"

@colinrotherham
Copy link
Contributor

colinrotherham commented Jun 12, 2023

Great stuff @colinrotherham - do you happen to know if any views are forming about the use of "Prettier".

I'm somewhat expecting that the release path for this will be us saying "these type of rules won't be checked any more, if you care about them use prettier, if you don't accept that part of your SCSS won't be linted anymore"

Yeah we're happy to apply formatting using Prettier for other languages

But I've now opened a PR to review all the changes to confirm we're all happy:

@colinrotherham
Copy link
Contributor

Pushed again with stylelint@15.9.0

@romaricpascal
Copy link
Member

Great stuff @colinrotherham - do you happen to know if any views are forming about the use of "Prettier".

Pretty happy to use Prettier for formatting on my side, whichever the language. I find the gain of consistency and not having to make decisions about the formatting outweighs any surprising format that may have been chosen by the tool (but which is usually thought through anyway). I quite welcome Stylelint going for separating itself from the formating and pushing towards Prettier.

@colinrotherham
Copy link
Contributor

Rebased to pick up #36

@colinrotherham
Copy link
Contributor

Rebased with latest package updates

@colinrotherham
Copy link
Contributor

Added the Dependabot security bump from:

Going to bring this out of draft too as it can be considered ready for review

@romaricpascal
Copy link
Member

romaricpascal commented Jul 14, 2023

If it's of interest, for people that may not want to move to Prettier, the stylistic rules that were removed from Stylelint 15 have been extracted by someone in their own stylelint-stylistic plugin, which gives another route for tackling formatting 😊

@kevindew
Copy link
Member Author

@romaricpascal Oh that's great.

Hmm I wonder if that can offer us a backwards compatible option. It'd be cool if we could say to people "As stylelint recommends using prettier we recommend that, however if you don't want to and don't want to lose any rules add the stylelint-stylistic plugin"

What I'm not clear on, with that option, is if we could have configuration for rules that target stylelint-stylistic and not have errors if the plugin is not enabled.

@colinrotherham
Copy link
Contributor

Would be great to write a CHANGELOG entry that says you're now free to use your own formatter

i.e. Either switch to stylelint-stylistic (easy) or adopt Prettier (hard)

Worth a check to see if anything in stylelint-stylistic conflicts with stylelint-config-gds@0.3.0

@kevindew
Copy link
Member Author

Would be great to write a CHANGELOG entry that says you're now free to use your own formatter

i.e. Either switch to stylelint-stylistic (easy) or adopt Prettier (hard)

Worth a check to see if anything in stylelint-stylistic conflicts with stylelint-config-gds@0.3.0

Yeah I'm expecting there's a couple of things that differ. I'm assuming (but haven't checked) that stylelint-stylistic follows stylelint-config-recommended - so I'd expect all the rules we're deleting in this PR are examples of deviations.

@romaricpascal
Copy link
Member

romaricpascal commented Jul 18, 2023

Yeah I'm expecting there's a couple of things that differ. I'm assuming (but haven't checked) that stylelint-stylistic follows stylelint-config-recommended - so I'd expect all the rules we're deleting in this PR are examples of deviations.

I had a quick look this morning and stylelint-config-recommended@9.0.0 has very little rules in terms of formatting code (that was a surprise). Asrules not in configurations are off by default, that means it took care of very little in terms of formatting.

stylelint-stylistic's configuration adds quite a few formating rules (65 more rules + 1 change from `stylelint-, if my diff is accurate).

Worth a check to see if anything in stylelint-stylistic conflicts with stylelint-config-gds@0.3.0

The formatting set by stylelint-stylistic configuration does conflict with the rules that were removed by 0bab4e7 :

  • 'block-closing-brace-newline-after' doesn't accound for specific at rules and is just set to always
  • 'number-leading-zero' is set to always rather than never

What I'm not clear on, with that option, is if we could have configuration for rules that target stylelint-stylistic and not have errors if the plugin is not enabled.

Unfortunately, Stylelint treats unknown rules as errors, so we do need to clean up the rules that were deprecated 😢

Nothing to stop the update, just inform what we can recommend. Maybe something in the line of:

Following Stylelint 15's deprecation of stylistic rules, this configuration no longer includes rules that would format your code.

We recommend you use Prettier to format your styles.

If setting up Prettier is not possible, you can add the stylelint-stylistic plugin to your configuration to recover the formatting rules that Stylelint deprecated. It'll be up to you to configure them, though, either by:

@romaricpascal
Copy link
Member

Looking neat to me and the changelog reads well as well 🙌🏻 @kevindew I'll leave you the final word on the extra changes (that rule for the property values look really useful) 😊

@colinrotherham
Copy link
Contributor

colinrotherham commented Jul 18, 2023

I've pushed up the star Stylelint 15 feature:

// Disallow unknown property and value pairs
// https://stylelint.io/user-guide/rules/declaration-property-value-no-unknown/
'declaration-property-value-no-unknown': true,

But it flags invalid CSS for Sass mixins and functions:

padding: govuk-spacing(2) 0;

Will do some more digging

@romaricpascal
Copy link
Member

romaricpascal commented Jul 18, 2023

That's an unfortunate side effect. Could do a two step update, maybe?

  1. release an update to 0.4.0 that enables Stylelint 15
  2. release a further 1.0.0 (or 0.5.0) update that enables that new rule once we figure how to make it play nice with Sass (there are options to configure what properties can accept: https://stylelint.io/user-guide/rules/declaration-property-value-no-unknown/#propertiessyntax--property-syntax- and https://stylelint.io/user-guide/rules/declaration-property-value-no-unknown/#typessyntax--type-syntax-).

@colinrotherham
Copy link
Contributor

@romaricpascal Let's remove it as it's not enabled by default yet in Stylelint

Yeah, it's quite nice to allow govuk-* mixins as values but not for this release

files: ['**/*.scss'],
rules: {
  // Disallow unknown property and value pairs (except govuk mixins and functions)
  // https://stylelint.io/user-guide/rules/declaration-property-value-no-unknown/
  'declaration-property-value-no-unknown': [
    true, {
      ignoreProperties: {
        '/.+/': '/govuk\\-/'
      }
    }
  ]
}

Would have to do something like:

1.0.0 — Breaking release to remove stylistic rules
2.0.0 — Breaking release to validate property-value pairs

With more investigation into the stylelint-scss rules

@romaricpascal
Copy link
Member

romaricpascal commented Jul 18, 2023

Two steps sound good to me! I think we should nudge people to experiment with that new rule (as it's super handy) to see how it breaks for them on their project and inform a possible shared configuration, hopefully one that'll work neatly with scss files as well 😊

@colinrotherham
Copy link
Contributor

Ready for review again 😊

All working brilliantly on GOV.UK Frontend

It's nice to see Sass formatting applied on save or commit (via Prettier)

@colinrotherham
Copy link
Contributor

Rebased to bring in #43 and updated to stylelint@15.10.2

Copy link
Member Author

@kevindew kevindew left a comment

Choose a reason for hiding this comment

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

Thanks so much for taking this further forward, it looks nearly ready to go. I've made some suggestions on the changelog - I feel like we need to emphasise further the impact of the loss of rules.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
css-rules.js Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
kevindew and others added 3 commits July 20, 2023 13:50
This updates the core stylelint dependency to stylelint 15.x and updates
the stylelint config versions to stylelint 15 compatible versions (I
picked the latest versions).
@colinrotherham colinrotherham changed the base branch from main to remove-deprecated-stylistic-rules July 20, 2023 12:52
Base automatically changed from remove-deprecated-stylistic-rules to main July 21, 2023 08:32
@colinrotherham
Copy link
Contributor

We're good to go on this. Thanks @kevindew for steering us through the process 😊

@colinrotherham colinrotherham merged commit 5cdee07 into main Jul 27, 2023
4 checks passed
@colinrotherham colinrotherham deleted the stylelint-15 branch July 27, 2023 11:06
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.

3 participants