fix: allow Ctrl+C to interrupt build-scss command - #4155
Conversation
✅ Deploy Preview for paragon-openedx-v23 ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Thanks for the pull request, @PKulkoRaccoonGang! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-23.x #4155 +/- ##
================================================
+ Coverage 94.39% 94.40% +0.01%
================================================
Files 242 242
Lines 4296 4306 +10
Branches 1020 981 -39
================================================
+ Hits 4055 4065 +10
Misses 237 237
Partials 4 4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
brian-smith-tcril
left a comment
There was a problem hiding this comment.
Thanks for doing this! This definitely is an improvement over the current state. I left a few comments with small cleanup suggestions, and one with a bigger question.
| process.on('SIGINT', () => { | ||
| // eslint-disable-next-line no-console | ||
| console.log(chalk.yellow('\nBuild interrupted. Partial output may exist in the output directory.')); | ||
| process.exit(130); | ||
| }); |
There was a problem hiding this comment.
Nothing else in this file uses console.log, and directly calling process.exit from the lib file seems less than ideal. We already have error handling in
paragon/bin/paragon-scripts.js
Lines 275 to 287 in c4e95f0
so it'd be nice to just surface the error from this function and hanve
bin/paragon-scripts handle it.
There was a problem hiding this comment.
Moved the SIGINT handler to bin/paragon-scripts.js. Unfortunately we can't surface SIGINT as a thrown error through the catch block because sass.compile() is synchronous, process.exit(130) in the signal handler is the only way to interrupt it immediately. So the handler lives in the entry point instead.
| // eslint-disable-next-line no-console | ||
| console.log(chalk.grey('Press Ctrl+C to interrupt the build (takes effect between compilations).\n')); |
There was a problem hiding this comment.
Same thing about trying to keep console.log out of this file. I figure for this one we can just remove these lines.
I'd also like to look into what it would take to make it so we can actually interrupt the compilation. I was thinking we might be able to use sass.compileAsync but the sass docs mention
⚠️ Heads up!
When using the sass npm package, compile is almost twice as fast as compileAsync, due to the overhead of making the entire evaluation process asynchronous.
There was a problem hiding this comment.
I also looked into using a worker thread to make the compilation truly interruptible. It would require a separate worker file with the sass importer logic duplicated, message passing for compilation results, and error handling across threads. It's more complexity than the current one-liner, and the benefit is just saving a few seconds on interruption...
There was a problem hiding this comment.
Yeah, it's definitely worth landing an improvement instead of blocking this on a complicated rework.
Maybe just showing a "cancelling" or "exiting" message would be helpful? Just some form of immediate feedback so users know their Ctrl+C was picked up.
There was a problem hiding this comment.
We're using the synchronous sass.compile, the event loop stays blocked during compilation, which means the SIGINT handler only gets a chance to run after the current theme finishes.
So even if we add an interrupt message, it wouldn't appear right away, it would show up once compilation of the current theme completes.
There was a problem hiding this comment.
It'd be nice to add a test to this file checking the SIGINT functionality.
brian-smith-tcril
left a comment
There was a problem hiding this comment.
I'm happy with this PR. It's a solid improvement over the current state of things, and considering the tradeoffs required to make sass.compile async I think it's the best option we have.
I left one last comment about changing the tests back to sync from async now that buildScssCommand is staying sync, once that's cleaned up I'll be happy to merge this!
| }); | ||
|
|
||
| it('should compile core and theme stylesheets', () => { | ||
| it('should compile core and theme stylesheets', async () => { |
There was a problem hiding this comment.
Now that buildScssCommand is back to not being async we can probably go back to a sync test to make the diff smaller
There was a problem hiding this comment.
Corrected, thanks
|
🎉 This PR is included in version 23.21.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Changed color.primary.base/light (light theme) and the full primary ramp + brand/text/body/header colors (dark theme) in the design token JSON sources, plus 6 hardcoded brand-tinted hex values found by hunt in themes/dark/_extras.scss, _discussion.scss and _learning.scss. Replaced logo/favicon assets with vivai's. NOTE on build method: 'npm run build' / 'make build' hangs indefinitely on this environment during the postCSS stage of the Light theme compilation (paragon build-scss), reproduced identically even with completely unmodified stock colors, so it is NOT caused by this change's content. Upgraded @openedx/paragon 23.14.9 -> 23.22.2 first (fixes a known forEach-without-await hang, openedx/paragon#4155) which did not resolve it either -- points to a deeper environment-specific issue with the postcss-combine-duplicated-selectors step on this machine, not yet root-caused. Workaround applied instead: restored edly-io's own pre-built dist/ CSS (already committed on this branch, produced by their CI) via 'git checkout HEAD -- dist/', then patched the compiled light/dark .css and .min.css directly with the same hex substitutions (see list below), rather than rebuilding via the broken local pipeline. Source maps (.css.map) were left untouched/stale as a result -- fine for production use, only affects devtools CSS source attribution. Substitutions applied: 15376D->808C48, F2F7F8->FAF3E0, AEC7F6->B5C48A, 8e9fbf->9AAB6E, 747e92->6B7A45, 5a6278->4F5A32, 292A2C->22201A, 777792->8A9463, CE80AA->D4903D, 112F6B->2B3318, 0D0D0E->1A1C12, 211E82->4F5A32, 6B7A94->6B7A5A, 717D8A->6B7A5A. If this needs a real source rebuild later (e.g. to add a new component override), retry 'make build' first -- the paragon version bump may combine with a future postcss-combine-duplicated-selectors fix to resolve it; if it still hangs, the same dist/-patching approach here can be reapplied.
Description
Issue: #3949
sass.compile()is a synchronous function. While it runs, Node.js is frozen - it can't do anything else, including responding to Ctrl+C. Instead of launching all compilations at once, we run them one at a time with asetTimeout(resolve, 0)pause between each.For example:
Merge Checklist
exampleapp?Post-merge Checklist