-
Notifications
You must be signed in to change notification settings - Fork 13
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 - Gulp: Support disabling sass sourcemaps #103
Conversation
- Drop `gulp-sourcemaps` which is seemingly neglected and is the reason for 3 npm audit warnings. Gulp v4 and v5 have native support for toggling sourcemaps. - Add new boolean setting `settings.compile.sassSourcemaps` that defaults to `true` but can be set to `false` in consumer projects to disable outputting sass sourcemaps. This addresses the sass portion of uswds#70. Exclusion of USWDS JS sourcemaps would need a separate setting and handling.
- Coerce settings.compile.sassSourcemaps to boolean to ensure the sourcemaps options receives a supported value (since this is user settable).
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.
Thanks for submitting this PR!
Verified setting works as expected. Created a test PR in uswds/uswds-sandbox#143.
Before | After |
---|---|
Dependencies
Before [develop]
14 vulnerabilities (3 moderate, 11 high)
After
Moderate vulnerabilities are gone.
11 high severity vulnerabilities
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.
Working like a charm! Thanks for your contribution
- No perceived regressions or build errors when installed on site
- Sourcemaps included by default
- Successfully disabled sourcemaps via
settings.compile.sassSourcemaps
@mahoneycm @mejiaj - Would you like me to resolve the merge conflicts since #96 was merged, or would you like to handle that? |
@mdmower-csnw if you're able to resolve with the latest that would help a lot! |
Thanks for reporting, we'll take a look. |
@mejiaj - I retested this PR after merging the latest from For a project where I use uswds 3.7.1...
I've gone ahead and merged upstream FYI, the merge strategy used in 77305e5 was as follows: # origin = https://github.com/mdmower-csnw/uswds-compile.git
# upstream = https://github.com/uswds/uswds-compile.git
git checkout csnw-sourcemaps
git reset --hard origin/csnw-sourcemaps
git fetch upstream
git merge upstream/develop
# git reports merge conflict in package-lock.json
git checkout upstream/develop package-lock.json
npm install
git add package-lock.json
git merge --continue |
gulp-sourcemaps
which is seemingly neglected and is the reason for 3 npm audit warnings. Gulp v4 and v5 have native support for toggling sourcemaps.settings.compile.sassSourcemaps
that defaults totrue
but can be set tofalse
in consumer projects to disable outputting sass sourcemaps.This addresses the sass portion of #70. Exclusion of USWDS JS sourcemaps would need a separate setting and handling.
Related issue
Closes #70