-
Notifications
You must be signed in to change notification settings - Fork 14
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
USWDS-Compile - Sass: Create quiet deprecations setting #131
base: develop
Are you sure you want to change the base?
Conversation
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.
This works great! Love that it won't need further updates for future deps and it honors custom warnings.
LGTM! 👍
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.
@mahoneycm I tested all three use cases you outlined. Confirming that this work fixes those use cases.
There was a little confusion in the first test case.
@mejiaj to for clarirty, are you seeing deprecation warnings on the In my instructions, I laid it out in three linear sections and marked where you would and would not see deprecation warnings. During my testing, I saw no deprecation warnings in the "1. Confirm USWDS deprecations are silenced" section. They should only begin to appear at the end of the next section 🤔 |
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.
LGTM! One small suggestion on the text in the README, but other than that, looking good!
- Confirm that this silences USWDS Sass deprecation warnings
- Confirmed project Sass still throws deprecation warnings
- Confirmed Sass still compiles as expected
- Confirmed that the
uswds.settings.compile.quietSassDeps
setting works as expected - Confirmed the README is updated and accurate
- Suggested a small tweak for clarity
Note
I needed to update the uswds branch in the Sandbox testing repo from step 3 to develop
since the POAM branch was already merged in.
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.
Adding one more suggestion for consideration. After reading through the doc updates, I'm feeling like some added clarity to the setting name might be helpful. Happy to talk about it you have any questions.
gulpfile.js
Outdated
@@ -66,6 +66,7 @@ let settings = { | |||
}, | |||
browserslist: ["> 2%", "last 2 versions", "IE 11", "not dead"], | |||
sassSourcemaps: true, | |||
quietSassDeps: true |
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: Wondering if we can improve clarity with the setting name here to highlight that this is about Sass "warnings", not just "deps" (This also clarifies that we are talking about deprecations, not dependencies).
Also wondering if it should be written in the negative, so that we can set "false" as a default. This part is very much a quibble and could just be personal preference, but it felt a bit odd to add a settings.compile.quietSassDeps = false
to the gulpfile.
Curious what you think!
quietSassDeps: true | |
sassDeprecationWarnings: false |
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.
Hmm I'm divided here. While I do think that the naming convention your suggesting makes sense, part of me thinks we should match what Sass is providing. Since quietDeps is the name of setting I see potential value in keeping ours close to it.
Do you see any value in changing it to sassQuietDeps
so we're sort of "prefixing" the Sass setting name with "Sass"?
Alternatively, we use this approach which would probably be more readable for users unfamiliar with this Sass deprecation 🤔
sassDeprecationWarnings: false
...
sass({
...
quietDeps: !settings.compile.sassDeprecationWarnings,
})
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.
Renamed in 7a4c434!
Co-authored-by: Amy Leadem <[email protected]>
I don't mean to rock the boat as this is an appreciated feature, but I think it's worth pointing out that there are other open issues that could be solved at the same time by simply allowing users to configure sass options. The approach I took in the fork we're using not only silences deprecations by default (i.e. good out of box experience), but it also:
|
@mdmower-csnw Thank you for sharing! We've decided to move forward with this approach for the time being but I've opened #133 for further discussion and to track this potential new feature. We appreciate your input and feedback as we improve Compile! |
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.
@mahoneycm just want to confirm... on the 1st test step, I am getting a deprication, although it is different from those I get in step 3. Here is a vid so you can see exactly what I'm talking about. :)
Screen.Recording.2024-12-11.at.4.14.52.PM.mov
Good call out @cathybaptista! That deprecation is separate from style definition deprecations and is being tracked by uswds/uswds#6103! We'll resolve that in upcoming work. It's good to know that users may still see that warning despite this PR. |
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.
@mahoneycm LGTM then. :)
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.
LGTM! Approving because the code looks good, but the PR description needs some updates (listed below).
Thanks for setting up these testing branch and instructions. Made checking everything very straightforward!
- Confirm that this silences USWDS Sass deprecation warnings by default
- Confirmed that the
uswds.settings.compile.sassDeprecationWarnings = true
turns USWDS deprecation warnings back on - Confirmed non-USWDS Sass triggers deprecation warnings
- Confirmed Sass still compiles as expected
- Confirmed the README is updated and accurate
- Confirmed the PR description is up-to-date and accurate
- There are outdated references to
settings.compile.quietSassDeps
. These should be updated tosettings.compile.sassDeprecationWarnings
- I needed to update the uswds branch in the Sandbox testing repo from step 3 to
develop
since the POAM branch was already merged in.
- There are outdated references to
Summary
Silences Sass deprecation warnings for downstream users while allowing local failures to still be triggered.
USWDS Sass deprecations are being tracked in uswds/uswds#6104
Based on work contributed by @mdmower-csnw. Thank you for your contribution!
Breaking change
This is not a breaking change.
Related issue
Closes #132
Related pull requests
Follow up to #126 and additional Sass deprecations popping up.
Documentation update: uswds/uswds-site#3006
Preview link
Test repo →
Problem statement
In its next major release, Sass is changing how it handles mixed declaration style definitions to match new CSS patterns. Because of this, users are getting alarming warnings during compilation.
The pattern is not currently deprecated and we are on track to update the related USWDS code.
Sass continues to push out new deprecation warnings in minor releases.
Solution
In
gulpfile.js
, createsassSilenceDeps
setting and set the default totrue
.This approach will mute USWDS warnings, including any future warnings that come up, while allowing warnings from custom sass to still be triggered in the terminal.
This will allow users to be made aware of impending deprecations without worrying about our own.
Users can decide to turn on the deprecation warning if they would like additional context on the upcoming deprecations.
Major changes
uswds.settings.compile.quietSassDeps
set totrue
by default.Testing and review
1. Confirm USWDS deprecations are silenced
On Sandbox
main
branch, install this version of compilenpm install "https://github.com/uswds/uswds-compile/tree/cm-quietDeps-setting" --save-dev
Run
npm run uswds:buildSass
Confirm there are no deprecation warnings
2. Confirm quietSassDeps setting is respected
After completing the steps above, visit
gulpfile.js
Under line
15
add the following:Run
npm run uswds:buildSass
Confirm deprecation now warnings appear in terminal
3. Confirm custom sass triggers deprecation warnings
npm install
npm run uswds:buildSass
_uswds-custom-styles.scss