fix(worker): preserve regexp options for process workers - #749
Conversation
🦋 Changeset detectedLatest commit: c8c8df5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #749 +/- ##
==========================================
- Coverage 97.89% 97.64% -0.26%
==========================================
Files 5 5
Lines 1665 1697 +32
Branches 640 652 +12
==========================================
+ Hits 1630 1657 +27
- Misses 35 39 +4
- Partials 0 1 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. WalkthroughThe plugin detects whether Node worker threads can be required and combines that result with minimizer capability settings. When worker threads are disabled, Priority: ⬇️ Low Merge Risk: 🔵 Low · up to The fallback currently preserves regular-expression options, but coverage does not directly protect the embedded-options case or force fallback dispatch on worker_threads-capable CI runtimes. These are bounded test-confidence risks; the PR is mergeable with follow-up awareness. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change affects worker selection rather than introducing a new external entrypoint or demonstrated privilege increase. No security finding is established, but worker execution across all runtime configurations has not been fully verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
test/parallel-option.test.jsOops! Something went wrong! :( ESLint: 10.10.0 Error: Error while loading rule 'jest/no-deprecated-functions': Unable to detect Jest version - please ensure jest package is installed, or otherwise set version explicitly Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/parallel-option.test.js (1)
248-248: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the unavailable-worker path on modern Node versions.
The guard skips the assertions whenever
worker_threadsloads. CI uses Node 12–24, where this module is available, so this test does not exercise the new plugin-level fallback on those runtimes. CI also pins Jest 27 or 29 for Node 10–16, so the Jest 30 premise does not apply to every supported runtime.Load
MinimizerPluginin an isolated module after mockingworker_threadsto throw. Keep the existing compile andworkerTransform/workerMinifyassertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 2969b16b-6795-4e0d-b36a-1534eaa2ec4a
📒 Files selected for processing (3)
src/index.jstest/parallel-option.test.jstypes/implementation.d.ts
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/implementation.test.js (1)
167-193: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd coverage for
RegExpvalues inembedded.options.When worker threads are disabled, an embedded
RegExpmust makecanMinifyByPathreturnfalse. Otherwisesrc/index.jsselectsworker.minify(options), whose process IPC serializer does not preserve regular expressions. The current tests cover onlyminimizer.options, so removing the embedded check would leave them passing.Suggested fix
+ it("should reject an embedded RegExp option when the worker cannot transfer it", () => { + expect( + canMinifyByPath( + { + minimizer: { implementation: [pathImpl] }, + embedded: { + implementation: pathImpl, + options: { comments: /license/i }, + claims: [], + offers: [], + at: [0], + }, + }, + { enableWorkerThreads: false }, + ), + ).toBe(false); + }); +
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: a1219ac1-fc53-40fb-8281-bb7642f6f17f
📒 Files selected for processing (1)
test/parallel-option.test.js
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
2 participants
Fallback to the serialized worker path when regular expression options are used with process workers that cannot preserve them. This keeps module-path minification on worker threads while fixing Node 10 CSS comment options.\n\nTests: implementation and parallel option suites (61 tests).
Summary by CodeRabbit