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

Updates Stylelint to 15.0.0 #298

Merged
merged 15 commits into from
Oct 5, 2023
Merged

Updates Stylelint to 15.0.0 #298

merged 15 commits into from
Oct 5, 2023

Conversation

Antonio-Laguna
Copy link
Member

@Antonio-Laguna Antonio-Laguna commented May 31, 2023

Related Issue/RFC: #284

Description of the Change

This updates stylelint to version 15.0.0 and decouple from @wordpress/stylelint-config. Upgrading to this should be painless.

Expanding a bit:

Stylelint 15 soft deprecates 76 stylistic rules of which we were leveraging 51 rules. Right now, any effort on @wordpress/stylelint-config is halted given they'll need to discuss what to do with those rules so forking from them seemed the shortest path that allows us to move forward.

This has been discussed on #295

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.
  • I have added a changeset to my PR. See CONTRIBUTING document for instructions

@Antonio-Laguna Antonio-Laguna requested a review from nicholasio May 31, 2023 11:23
@Antonio-Laguna Antonio-Laguna self-assigned this May 31, 2023
@changeset-bot
Copy link

changeset-bot bot commented May 31, 2023

🦋 Changeset detected

Latest commit: f68c04e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@10up/stylelint-config Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Antonio-Laguna
Copy link
Member Author

@nicholasio after the upgrade I can go and check the lining once we upgrade the packages

@nicholasio
Copy link
Member

Any idea why the test failed on node 16?

@Antonio-Laguna
Copy link
Member Author

@nicholasio failed on linting. Only lints on Node 16. Checking the issue, what fails linting is stylelint for the 10up-theme. I think after things are published I can go and update everything there?

# Conflicts:
#	package-lock.json
#	packages/stylelint-config/package.json
@nicholasio
Copy link
Member

nicholasio commented Sep 25, 2023

The failure is legit and not specific to node 16

image

We only run linting in one of the jobs and apparently, our deps update is forcing everything to be react 18 now.

I'm wondering if we should just make this a warning instead of an error.

@@ -50,7 +51,7 @@
"peerDependencies": {
"@babel/core": "^7.21.4",
"@babel/eslint-parser": "^7.21.3",
"@wordpress/eslint-plugin": "^13.10.0",
"@wordpress/eslint-plugin": "^14.9.0",
Copy link
Member

Choose a reason for hiding this comment

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

I believe this is forcing react 18

@Antonio-Laguna
Copy link
Member Author

These new errors aren't what they used to be! See https://github.com/10up/10up-toolkit/actions/runs/5738199032/job/15551300090#step:8:76

That said, I'm wondering if we should upgrade to React 18

@@ -1,5 +1,5 @@
// Test comment
@import "./components/sass-component";
@import url("./components/sass-component.scss");
Copy link
Member Author

Choose a reason for hiding this comment

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

@nicholasio thoughts here? I think being consistent between scss and no scss is the way to go. Doesn't seem possible to have one linting rule just for scss rules so it's one or the other.

Copy link
Member

Choose a reason for hiding this comment

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

fine by me!

nicholasio
nicholasio previously approved these changes Oct 4, 2023
@@ -1,5 +1,5 @@
// Test comment
@import "./components/sass-component";
@import url("./components/sass-component.scss");
Copy link
Member

Choose a reason for hiding this comment

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

fine by me!

@nicholasio nicholasio merged commit 3fce625 into develop Oct 5, 2023
5 checks passed
@nicholasio nicholasio deleted the feature/stylelint-upgrade branch October 5, 2023 14:56
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.

2 participants