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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: Upgrade to Stylelint 14 馃帀 #254

Merged
merged 6 commits into from Oct 10, 2021
Merged

feat!: Upgrade to Stylelint 14 馃帀 #254

merged 6 commits into from Oct 10, 2021

Conversation

adalinesimonian
Copy link
Member

@adalinesimonian adalinesimonian commented Oct 8, 2021

Closes #233

  • Stylelint dependency changed to point to v14 branch on GitHub (Will need to be changed to the packaged version once Stylelint 14 is published to npm.)
  • Default values for validate and snippet options set to ['css', 'postcss']
  • Removed syntax option
  • Removed configOverrides option
  • Documented changes and provided migration path in readme
  • Made appropriate changes in tests
  • Removed tests for removed features

Closes #233

- Stylelint dependency changed to point to v14 branch on GitHub (Will
  need to be changed to the packaged version once Stylelint 14 is
  published to npm.)
- Default values for `validate` and `snippet` options set to
  `['css', 'postcss']`
- Removed `syntax` option
- Removed `configOverrides` option
- Documented changes and provided migration path in readme
- Made appropriate changes in tests
- Removed tests for removed features
@adalinesimonian adalinesimonian linked an issue Oct 8, 2021 that may be closed by this pull request
6 tasks
Copy link
Member

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

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.

Same, looks good to me, will install this v1 branch over the weekend to give it a run, thanks a bunch @adalinesimonian, brilliant work 馃挴

Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

Excellent stuff, thank you!

I've suggested:

  • two minor documentation changes
  • not using some of the custom syntaxes

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
package.json Outdated
Comment on lines 166 to 167
"@stylelint/postcss-css-in-js": "^0.37.2",
"@stylelint/postcss-markdown": "^0.36.2",
Copy link
Member

@jeddy3 jeddy3 Oct 8, 2021

Choose a reason for hiding this comment

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

I don't think either of these packages are compatible with PostCSS@8.

The markdown and HTML packages will also be deprecated in favour of none-scoped names as we only scoped them as we thought access to the unscoped versions was lost but we access the packages now. Probably best sticking with postcss-scss for tests. Or at least skipping the HTML and markdown tests until the unscoped and PostCSS@8-compatible versions are ready.

And the monolithic CSS-in-JS package is going to be deprecated in favour of packages that focus on single CSS-in-JS libraries, like styled-component, Stitches or one of the other fifty plus.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've temporarily removed html and markdown tests in b94dca9. What would be a good replacement package for the css-in-js tests?

Copy link
Member

@jeddy3 jeddy3 Oct 9, 2021

Choose a reason for hiding this comment

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

Unfortunately, there isn't one right now. We're in a chicken and egg situation where we need new syntax packages for version 14 but people need version 14 to write those new syntax packages.

I'm going to dig into custom syntax compatibility for stylelint/stylelint#5563, as I'm surprised the tests passed here. Perhaps some of the existing packages are more compatible than I thought. I'll keep you posted.

Copy link
Member

Choose a reason for hiding this comment

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

It turns out that these syntaxes are more compatible with PostCSS@8 than we thought.

However, postcss-html and postcss-markdown are currently unmaintained. If you do use them in your tests, use the unforked version of postcss-markdown as we'll be deprecating @stylelint/postcss-markdown.

We won't be deprecating @stylelint/postcss-css-in-js until there are alternative syntaxes (specific to individual CSS-in-JS libraries) available. So you can continue to use it in your tests for now.

@adalinesimonian adalinesimonian self-assigned this Oct 8, 2021
@adalinesimonian adalinesimonian added pr: dependencies relates to dependencies type: refactor an improvement to the code structure upstream relates to an upstream package labels Oct 8, 2021
@adalinesimonian adalinesimonian added this to the v1.0.0 milestone Oct 8, 2021
TODO: Restore once postcss-html and postcss-markdown are PostCSS 8
compatible.

re: #254 (comment)
@adalinesimonian
Copy link
Member Author

adalinesimonian commented Oct 8, 2021

Slight tangent: I've opened an issue on the Stylelint repo (stylelint/stylelint#5580, tracked here in #256) for type definitions issues upstream that I encountered while working on v1. In the meantime, I am using workarounds in this repository.

Update: Potential fix up in stylelint/stylelint#5582

Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

Excellent stuff!

@adalinesimonian adalinesimonian merged commit a4e0454 into v1 Oct 10, 2021
@adalinesimonian adalinesimonian deleted the v1-stylelint-14 branch October 10, 2021 17:31
@adalinesimonian adalinesimonian added For Milestone Issue and removed pr: dependencies relates to dependencies upstream relates to an upstream package type: refactor an improvement to the code structure labels Oct 28, 2021
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.

Prepare for stylelint version 14
4 participants