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

Upgrade to Stylelint 15 and handle breaking changes (OSOE-678) #70

Closed
sarahelsaig opened this issue Aug 29, 2023 · 7 comments · Fixed by #85
Closed

Upgrade to Stylelint 15 and handle breaking changes (OSOE-678) #70

sarahelsaig opened this issue Aug 29, 2023 · 7 comments · Fixed by #85
Assignees

Comments

@sarahelsaig
Copy link
Member

sarahelsaig commented Aug 29, 2023

Followup to #67 (comment)

Updating stylelint to 15.10.3 from 14.14.0 would mean a major update that contains the following (breaking) changes:
https://stylelint.io/migration-guide/to-15

To sum up, they deprecated 76 rules because they are mostly styling related rules that can be achieved by pretty printers like Prettier so they won't support it anymore from the next major update.

These are the rules that we are using and deprecated:

'declaration-block-semicolon-newline-before': 'never-multi-line',
'function-comma-newline-after': 'always-multi-line',
'indentation': 4,
'max-line-length': 150,
'selector-list-comma-space-after': 'always-single-line',
'selector-list-comma-newline-after': 'always-multi-line',
'value-list-comma-newline-before': 'never-multi-line'

Consider how to handle these breaking rules and evaluate if using Prettier is a good fit.

Jira issue

@github-actions github-actions bot changed the title Upgrade to Stylelint 15 and handle breaking changes Upgrade to Stylelint 15 and handle breaking changes (OSOE-678) Aug 29, 2023
@tteguayco tteguayco self-assigned this Oct 31, 2023
@wAsnk
Copy link
Member

wAsnk commented Nov 3, 2023

@tteguayco are you actively working on this? If not I would start this if it is okay for you as well @Piedone @DemeSzabolcs

@tteguayco
Copy link

@tteguayco are you actively working on this? If not I would start this if it is okay for you as well @Piedone @DemeSzabolcs

No problem on my side, @wAsnk

@Piedone
Copy link
Member

Piedone commented Nov 3, 2023

Fine by me too.

@wAsnk
Copy link
Member

wAsnk commented Nov 5, 2023

It seems like the best fit is to use Prettier for this. Should I only handle scss style checking like it was before so it gives a warning on a styling violation or should I utilize all its features?

What other features does it have?
It can automatically fix styling after implementation (on build with CLI or pre-commit hook) the following languages that you can find in the parser selector here: https://prettier.io/playground/. The only customizable options are there on this playground site, so you can check those as well. Why only those options are available? TLDR; for unity, but you can read their philosophy about it here: https://prettier.io/docs/en/option-philosophy.
I checked for scss it has everything that we need currently and it styles the code like we want to.

It can be integrated with linters (eslint, styleint) to handle these styling options.

@Piedone
Copy link
Member

Piedone commented Nov 5, 2023

I think we need the following:

  • Make Prettier check the formatting when the NPM build:styles, compile:styles or lint:styles tasks are run (thus also when watch:styles), and produce an MSBuild build warning (that'll fail the build in CI builds), I.e. the same way Stylelint does it today but now we'd run both Stylint and Prettier.
  • If fixes can be done automatically when running these tasks in a way that it won't overwrite any pending edits you may have in your editor, then let's also do that. Otherwise, document how auto-fixing can be utilized.
  • Document everything in Styles.md, including how to configure Prettier (what little you can) and how to utilize it during development from your IDE.

We have JS, MD, and to an extent HTML linting (with the UI Testing Toolbox) covered, so we don't need anything else from Prettier at this stage under this issue. YAML will perhaps be needed under #11.

@wAsnk
Copy link
Member

wAsnk commented Nov 7, 2023

If fixes can be done automatically when running these tasks in a way that it won't overwrite any pending edits you may have in your editor, then let's also do that. Otherwise, document how auto-fixing can be utilized.

Do you mean not commented changes, or what do you mean by pending edits?

We have JS, MD, and to an extent HTML linting (with the UI Testing Toolbox) covered, so we don't need anything else from Prettier at this stage under this issue. YAML will perhaps be needed under #11.

Prettier only covers formatting rules, not code-quality rules so it is a bit different from these linters. https://prettier.io/docs/en/comparison

If we will use Prettier then we will need to turn off all the formatting rules for styles, that's what Prettier docs are suggesting:
https://github.com/prettier/stylelint-prettier#disabling-rules-that-may-conflict-with-prettier

Then I will do this for scss files only if it is not needed for others.

@Piedone
Copy link
Member

Piedone commented Nov 7, 2023

Do you mean not commented changes, or what do you mean by pending edits?

Unsaved edits that you have in an editor window. E.g. what shouldn't happen is that you save an SCSS file, watch:styles starts to build it, then you notice a typo and edit the file again but before you can save it Prettier overwrites your changes with auto-fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants