Ensure source maps are not included in built plugins#100
Ensure source maps are not included in built plugins#100agibson-godaddy merged 6 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses an issue where source maps were still being included in production builds despite attempts to strip them. Instead of fixing the regex pattern that strips sourcemap comments, the implementation now prevents sourcemap generation entirely during build and deploy tasks.
- Removes the regex-based sourcemap comment stripping from the copy task
- Conditionally generates sourcemaps only when not running build/deploy tasks
- Adds a new
isBuildTask()helper method to detect build-related CLI invocations
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tasks/copy.js | Removes the regex pattern that was attempting to strip sourcemap comments |
| tasks/compile.js | Conditionally wraps sourcemap generation with isBuildTask() checks for all asset types |
| pipes/scripts.js | Updates the JavaScript compilation pipeline to conditionally generate sourcemaps |
| lib/sake.js | Adds the isBuildTask() helper method to detect build-related CLI arguments |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
hey there! 👋 any ETA when this could be merged? looks good to me |
I need an approval. 😅 I'll chase it up! 👋 |
|
hey @agibson-godaddy! thank you that would really hep! say hi to the team from me |
There was a problem hiding this comment.
Left a comment with one fix, otherwise QA checks out!
One thing I noticed is that when running sake build, the sourcemap comment(s) are removed from the plugin's minified CSS and JS (unexpected?) as well as the copies in the build directory (expected).
I'm guessing that's a side effect of the build process, i.e. 1. remove the sourcemaps, 2. copy the files to the build directory. Question is: is the sourcemaps being removed in both sets of files expected and/or acceptable?
p.s. Hiiii @unfulvio!
Co-authored-by: Drew Jaynes <113622064+ajaynes-godaddy@users.noreply.github.com>
Yeah that's expected because we just don't generate them at all anymore. (So they're not being removed exactly, just never created to begin with.) We also don't commit minified CSS or sourcemaps so there's no risk of accidentally overwriting committed data in the repo. If someone wants the sourcemaps for development they can just run Thanks for the review!! |
|
thank you! ❤️ |
Issue: https://godaddy-corp.atlassian.net/browse/MWC-18446
Summary
Our intention has always been to not ship sourcemap files in production code, and as part of that we attempt to strip the sourcemap comments from minified files.
At some point the comment style must have changed and it was no longer getting stripped. I think they went from
/*style comments to//style in JS, and that accounts for the change.Rather than accounting for the new comment format I opted to implement the
TODOmethod of not generating them at all if we're in thebuildordeploytasks.QA
sake compile/*# sourceMappingURL=wc-customer-order-csv-export-admin.min.css.map *///# sourceMappingURL=wc-customer-order-csv-export-admin.min.js.mapsake buildbuild/directorybuild/directory