feat: add support for logical assignment operators in environment config#21219
Conversation
🦋 Changeset detectedLatest commit: 292b069 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #21219 +/- ##
=======================================
Coverage 92.77% 92.78%
=======================================
Files 591 591
Lines 64458 64487 +29
Branches 17908 17920 +12
=======================================
+ Hits 59802 59833 +31
+ Misses 4656 4654 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Merging this PR will degrade performance by 77.16%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | benchmark "lodash", scenario '{"name":"mode-development-rebuild","mode":"development","watch":true}' |
128.4 KB | 858.6 KB | -85.05% |
| ❌ | Memory | benchmark "side-effects-reexport", scenario '{"name":"mode-development-rebuild","mode":"development","watch":true}' |
133.3 KB | 859.1 KB | -84.48% |
| ❌ | Memory | benchmark "wasm-modules-sync", scenario '{"name":"mode-development-rebuild","mode":"development","watch":true}' |
130 KB | 253 KB | -48.62% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing bjohansebas:feat/environment-logical-assignment (292b069) with main (6e5bd5d)
alexander-akait
left a comment
There was a problem hiding this comment.
Reviewed the change end-to-end. The core transformation looks correct: target ||= value is semantically equivalent to the previous target = target || value for every call site here (all targets are member accesses on plain objects/arrays/globals, so the "evaluated twice" / falsy-value concerns don't bite), and the object-literal extraction in the chunk-loading modules preserves the exact emitted formatting. A couple of non-blocking observations, plus two inline notes:
-
lib/library/UmdLibraryPlugin.js(accessorAccess) still emits${a} = ${a} || {}for the namespace walk and wasn't migrated toassignOr. Is that intentional (UMD aiming for maximal compatibility), or just missed? If intentional, fine — just flagging the inconsistency. -
The browserslist version table and the
node/esXmappings line up with the ES2021 / Chromium-85 baseline, so those look right.
Main suggestion is the missing end-to-end test for the ||= branch (see inline comment on the unit test).
Generated by Claude Code
| }); | ||
| }); | ||
|
|
||
| describe("RuntimeTemplate.assignOr", () => { |
There was a problem hiding this comment.
This unit test covers assignOr in isolation, but the seven runtime modules that now call it (jsonp/import-scripts/module/read-file/require/wasm chunk loaders, share + css registry) are only exercised with the default logicalAssignment: undefined, i.e. the fallback branch. Nothing asserts that the ||= branch is actually emitted and produces valid runtime. Consider adding a configCase that builds with output.environment.logicalAssignment: true and checks the generated runtime contains ||=, so a future regression in any of those templates is caught end-to-end.
Generated by Claude Code
| * @param {string} value default value expression | ||
| * @returns {string} assignment expression | ||
| */ | ||
| assignOr(target, value) { |
There was a problem hiding this comment.
Worth noting in the JSDoc (or just keeping in mind for future callers): assignOr only models the ||/||= case, so it must never be used where target can hold a legitimate falsy value (0, "", false) — it would overwrite it. All current call sites default to objects/arrays, so this is safe today, but the helper name is generic enough that a later caller could trip on it. A one-line caveat in the doc comment would prevent that.
Generated by Claude Code
|
Fast review from Claude, other let’s rebase, thanks for this PR |
…figuration
- Updated WebpackOptions.json schema to include `logicalAssignment` property indicating support for logical assignment operators ('a ||= b', 'a &&= b', 'a ??= b').
- Modified Defaults.unittest.js to include tests for the new `logicalAssignment` property, ensuring it is correctly handled in various scenarios.
- Enhanced RuntimeTemplate with `assignOr` method to utilize logical assignment when supported, providing a fallback for environments that do not support it.
- Updated snapshot tests to reflect changes in logical assignment handling across various configurations.
- Extended types.d.ts to declare the new `logicalAssignment` property in the Environment interface and added corresponding methods in RuntimeTemplate.
c4d096c to
5c1426e
Compare
|
done the rebase |
|
Looks like some tests failed, let’s fix and we can merge |
|
This PR is packaged and the instant preview is available (12b8106). Install it locally:
npm i -D webpack@https://pkg.pr.new/webpack@12b8106
yarn add -D webpack@https://pkg.pr.new/webpack@12b8106
pnpm add -D webpack@https://pkg.pr.new/webpack@12b8106 |
Summary
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Logical_OR_assignment
What kind of change does this PR introduce?
Did you add tests for your changes?
Does this PR introduce a breaking change?
If relevant, what needs to be documented once your changes are merged or what have you already documented?
Use of AI