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
Remove syntax option #5297
Remove syntax option #5297
Conversation
I've:
I've commented out the PostCSS@8-incompatible syntax tests for each rule. We can reinstate them if the existing syntaxes are made compatible or move them to leaner and more specific syntaxes if they are written. Once this pull request is merged, we can start work on all the other release issues including adding more messages to ease updating. The tests will pass once stylelint/jest-preset-stylelint#32 is merged and released. |
This pull request is now ready for review. |
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[suggestion] It seems that we can remove this file: ⬇️
Lines 1 to 5 in b01ed25
declare module '@stylelint/postcss-css-in-js'; | |
declare module '@stylelint/postcss-markdown'; | |
declare module 'postcss-html'; | |
declare module 'postcss-sass'; | |
declare module 'sugarss'; |
Also for
|
I've removed the redundant types. |
package-lock.json
Outdated
@@ -21531,10 +21797,68 @@ | |||
"resolved": "https://registry.npmjs.org/style-search/-/style-search-0.1.0.tgz", | |||
"integrity": "sha1-eVjHk+R+MuB9K1yv5cC/jhLneQI=" | |||
}, | |||
"stylelint": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hope we won't have problems, that this stylelint
will be always behind. “Thanks”, npm 7.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where's this entry coming from, is it the jest-preset? We can set the peer to 14 when it's out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, peerDependency of jest-preset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we remove the peer dependency? It's odd having stylelint installed in stylelint automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do it as a patch release of the preset. We probably don't want an old version of stylelint (and postcss) hanging around as we migrate to PostCSS 8 it adds a bit of an unknown variable into the mix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think anyone uses jest-preset to test stylelint plugin without stylelint installed. So it's probably safe to remove peer dependency.
Co-authored-by: Aleks Hudochenkov <aleks@hudochenkov.com>
Co-authored-by: Aleks Hudochenkov <aleks@hudochenkov.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeddy3 Thank you so much for your great work! 👏🏼
Changelog:
@ybiquitous No worries. Thank you for your spot-on suggestions and reviews! |
Closes #5289
A work-in-progress pull request showing the removal of the:
syntax
option in favour ofcustomSyntax
The rule tests reliant on the
syntax
option will fail until the Jest preset is updated and the tests themselves moved tocustomSyntax
. Thecolor-function-notation
rule is done, and we can do the rest once #5295 and #5296 are merged.This would be the third pull request for the
v14
branch.