-
Notifications
You must be signed in to change notification settings - Fork 151
feat: support postcss-load-config 4 #520
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
Conversation
@@ -118,7 +118,7 @@ | |||
"coffeescript": "^2.5.1", | |||
"less": "^3.11.3 || ^4.0.0", | |||
"postcss": "^7 || ^8", | |||
"postcss-load-config": "^2.1.0 || ^3.0.0", | |||
"postcss-load-config": "^2.1.0 || ^3.0.0 || ^4.0.0", |
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.
Hi there,
I am a bit confused about this change.
In my opinion the change should be like this.
"postcss-load-config": ">=2.1.0
Because in current state we are matching everything v3.0.0
and v4.0.0
(this can be effectively be done by ^2.1.0
) which then can again break in v5.0.0
So in my opinion matching everything above and including v2.1.0
is the best approach :)
Thanks for reading :D
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 guess you mean >=2.1.0
according to semver, as ^2.1.0
only matches anything version 2 (version 3 is out of scope). I think either way is fine and it's up to the maintainers preference, as there are pros/cons for both approach
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 guess you mean >=2.1.0 according to semver, as ^2.1.0 only matches anything version 2 (version 3 is out of scope). I think either way is fine and it's up to the maintainers preference, as there are pros/cons for both approach
Forgive my mistake and that is exactly what i mean :D
Thanks for correcting me
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.
Yeah, this feels like a trade-off to me, and I don't have a strong opinion either way. If you only mention version 2, 3, and 4, then you need to manually add 5 once that's released and you make sure that its breaking changes don't affect you. If you use >=2.1.0
, you're asserting that this will work regardless of any breaking changes postcss-load-config
might make in the future, which might be fine for a while, until one day it bites some user in potentially a really confusing way.
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.
@Conduitry Thanks for giving your opinion on this matter :)
Have a good day :)
Fix #519
Checking the code changes in postcss-load-config, this should be enough to suppress peer dep issues.
Before submitting the PR, please make sure you do the following
npm run lint
!)Tests
npm test
oryarn test