feat: enable web binary template by default in WebEncodePlugin#2545
Conversation
PupilTong
requested review from
Sherry-hue,
colinaaa,
luhc228 and
upupming
as code owners
🦋 Changeset detectedLatest commit: 5290d99 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBinary template encoding is now the default for web templates; only ChangesWeb binary template defaulting
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/enable-web-binary-template-by-default.md:
- Around line 18-20: The markdown link text has a missing space in
"`@lynx-js/web-core`Changelog"; update the link text to include a space so it
reads "`@lynx-js/web-core` Changelog" (locate and edit the text in the
.changeset/enable-web-binary-template-by-default.md diff block containing the
link).
In `@packages/webpack/template-webpack-plugin/src/WebEncodePlugin.ts`:
- Around line 108-111: The branch in WebEncodePlugin using
isExperimentalWebBinary includes an impossible numeric comparison
(isExperimentalWebBinary === 0); since process.env values are string|undefined,
remove the numeric comparison and only compare string values (e.g., 'false' and
'0') or handle undefined explicitly; update the conditional around
isExperimentalWebBinary in the WebEncodePlugin method to drop the === 0 check so
the unreachable clause is eliminated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: be1ad1a1-bac5-475b-9677-2d837c743447
📒 Files selected for processing (4)
.changeset/enable-web-binary-template-by-default.mdpackages/web-platform/web-core-e2e/scripts/generate-build-command.jspackages/web-platform/web-tests/package.jsonpackages/webpack/template-webpack-plugin/src/WebEncodePlugin.ts
💤 Files with no reviewable changes (1)
- packages/web-platform/web-core-e2e/scripts/generate-build-command.js
PupilTong
force-pushed
the
p/hw/opt-out-wasm
branch
from
907e383 to
31a0800
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Merging this PR will improve performance by 18.85%
Performance Changes
Comparing 
Footnotes
|
React Example#7803 Bundle Size — 225.52KiB (0%).5290d99(current) vs 5f3b6eb main#7798(baseline) Bundle metrics
|
| Current #7803 |
Baseline #7798 |
|
|---|---|---|
 Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
180 |
180 |
| Duplicate Modules |
69 |
69 |
| Duplicate Code |
44.54% |
44.54% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #7803 |
Baseline #7798 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
79.77KiB |
79.77KiB |
Bundle analysis report Branch PupilTong:p/hw/opt-out-wasm Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#9376 Bundle Size — 900.03KiB (0%).5290d99(current) vs 5f3b6eb main#9371(baseline) Bundle metrics
|
| Current #9376 |
Baseline #9371 |
|
|---|---|---|
| Initial JS |
44.46KiB |
44.46KiB |
| Initial CSS |
2.22KiB |
2.22KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
9 |
9 |
| Assets |
11 |
11 |
 Modules |
229(+0.44%) |
228 |
| Duplicate Modules |
11 |
11 |
| Duplicate Code |
27.28%(-0.04%) |
27.29% |
| Packages |
10 |
10 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #9376 |
Baseline #9371 |
|
|---|---|---|
| JS |
495.9KiB |
495.9KiB |
| Other |
401.92KiB |
401.92KiB |
| CSS |
2.22KiB |
2.22KiB |
Bundle analysis report Branch PupilTong:p/hw/opt-out-wasm Project dashboard
Generated by RelativeCI Documentation Report issue
React External#918 Bundle Size — 680.82KiB (0%).5290d99(current) vs 5f3b6eb main#913(baseline) Bundle metrics
|
| Current #918 |
Baseline #913 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
3 |
3 |
| Modules |
17 |
17 |
| Duplicate Modules |
5 |
5 |
| Duplicate Code |
8.59% |
8.59% |
| Packages |
0 |
0 |
| Duplicate Packages |
0 |
0 |
Bundle analysis report Branch PupilTong:p/hw/opt-out-wasm Project dashboard
Generated by RelativeCI Documentation Report issue
React Example (Element Template)#2 Bundle Size — 198.61KiB (0%).ef964b5(current) vs a317fd6 main#1(baseline) Bundle metrics
|
| Current #2 |
Baseline #1 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
78(+1.3%) |
77 |
| Duplicate Modules |
23 |
23 |
| Duplicate Code |
40.71%(-0.05%) |
40.73% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle analysis report Branch PupilTong:p/hw/opt-out-wasm Project dashboard
Generated by RelativeCI Documentation Report issue
React MTF Example#935 Bundle Size — 196.68KiB (0%).5290d99(current) vs 5f3b6eb main#930(baseline) Bundle metrics
|
| Current #935 |
Baseline #930 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
3 |
3 |
| Modules |
174 |
174 |
| Duplicate Modules |
66 |
66 |
| Duplicate Code |
44.05% |
44.05% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #935 |
Baseline #930 |
|
|---|---|---|
| IMG |
111.23KiB |
111.23KiB |
| Other |
85.45KiB |
85.45KiB |
Bundle analysis report Branch PupilTong:p/hw/opt-out-wasm Project dashboard
Generated by RelativeCI Documentation Report issue
PupilTong
force-pushed
the
p/hw/opt-out-wasm
branch
from
ef964b5 to
4b09aa2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.changeset/enable-web-binary-template-by-default.md (1)
18-18:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWording "could support" still present — not fully addressed from previous round.
Line 18 still reads grammatically incorrect: "could support the new output format".
📝 Proposed fix
-**Upgrade to `@lynx-js/web-core@0.20.2` could support the new output format** +**Upgrade to `@lynx-js/web-core@0.20.2` to support the new output format**🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.changeset/enable-web-binary-template-by-default.md at line 18, The phrase "could support the new output format" is grammatically weak; update the sentence containing "could support the new output format" to a definitive form such as "supports the new output format" or "now supports the new output format" so the changelog reads clearly and confidently; locate the literal string "could support the new output format" in .changeset/enable-web-binary-template-by-default.md and replace it with the chosen corrected wording.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/enable-web-binary-template-by-default.md:
- Line 2: The changeset currently marks "@lynx-js/template-webpack-plugin" as
"patch" despite a behavioral default change (feat) that flips output from JSON
to binary; update the changeset entry by changing the version bump for
"@lynx-js/template-webpack-plugin" from "patch" to "minor" so the release semver
reflects a non-patch behavioral change and aligns with the commit subject.
---
Duplicate comments:
In @.changeset/enable-web-binary-template-by-default.md:
- Line 18: The phrase "could support the new output format" is grammatically
weak; update the sentence containing "could support the new output format" to a
definitive form such as "supports the new output format" or "now supports the
new output format" so the changelog reads clearly and confidently; locate the
literal string "could support the new output format" in
.changeset/enable-web-binary-template-by-default.md and replace it with the
chosen corrected wording.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 15172207-f3a2-410e-b1de-8c370a33389b
📒 Files selected for processing (4)
.changeset/enable-web-binary-template-by-default.mdpackages/web-platform/web-core-e2e/scripts/generate-build-command.jspackages/web-platform/web-tests/package.jsonpackages/webpack/template-webpack-plugin/src/WebEncodePlugin.ts
💤 Files with no reviewable changes (1)
- packages/web-platform/web-core-e2e/scripts/generate-build-command.js
React Example with Element Template#69 Bundle Size — 198.12KiB (0%).5290d99(current) vs 5f3b6eb main#64(baseline) Bundle metrics
|
| Current #69 |
Baseline #64 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
78 |
78 |
| Duplicate Modules |
23 |
23 |
| Duplicate Code |
40.57%(-0.02%) |
40.58% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #69 |
Baseline #64 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
52.36KiB |
52.36KiB |
Bundle analysis report Branch PupilTong:p/hw/opt-out-wasm Project dashboard
Generated by RelativeCI Documentation Report issue
PupilTong
force-pushed
the
p/hw/opt-out-wasm
branch
from
4b09aa2 to
93c37b9
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
.changeset/enable-web-binary-template-by-default.md (2)
2-2:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winBump should likely be
minor, notpatch.This changeset flips the default web template encoding from JSON to Binary — a user-observable behavior change for every consumer that doesn't set
EXPERIMENTAL_USE_WEB_BINARY_TEMPLATE. SemVer-wise that's a new feature/behavior change and warrants aminorbump rather thanpatch, especially given thefeat(web):subject.📝 Proposed change
-"@lynx-js/template-webpack-plugin": patch +"@lynx-js/template-webpack-plugin": minor🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.changeset/enable-web-binary-template-by-default.md at line 2, The changeset currently marks "@lynx-js/template-webpack-plugin" as a patch but the change flips default web template encoding (user-visible) so update the changeset to use a minor bump instead of patch; in the .changeset entry that contains the line "@lynx-js/template-webpack-plugin": patch change "patch" to "minor" and ensure the changeset title/summary reflects the feat(web): behavior change so the release tooling will produce a minor release.
18-18:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAwkward phrasing in upgrade note.
"could support" reads oddly here; it's an instruction to upgrade so the new format is supported. Minor wording fix:
📝 Proposed fix
-**Upgrade to `@lynx-js/web-core@0.20.2` could support the new output format** +**Upgrade to `@lynx-js/web-core@0.20.2` to support the new output format**🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.changeset/enable-web-binary-template-by-default.md at line 18, The upgrade note sentence "**Upgrade to `@lynx-js/web-core@0.20.2` could support the new output format**" is awkward; change that phrase to a clearer instruction such as "**Upgrade to `@lynx-js/web-core@0.20.2` to support the new output format**" or "**Upgrade to `@lynx-js/web-core@0.20.2` to enable support for the new output format**" by editing the same line in the changeset.
🧹 Nitpick comments (1)
packages/webpack/template-webpack-plugin/src/WebEncodePlugin.ts (1)
108-135: 💤 Low valueOpt-out check is now correctly string-only.
The conditional only matches
'false'/'0'(both validprocess.envstring values), and all other states — including unset — fall through to the binary path, which matches the new default documented in the changeset.One small consideration: the comparison is case-sensitive, so
EXPERIMENTAL_USE_WEB_BINARY_TEMPLATE=False(orFALSE) would silently take the binary path rather than the intended JSON opt-out. If you want to be lenient toward common user inputs:♻️ Optional case-insensitive opt-out
- const isExperimentalWebBinary = process - .env['EXPERIMENTAL_USE_WEB_BINARY_TEMPLATE']; - if ( - isExperimentalWebBinary === 'false' - || isExperimentalWebBinary === '0' - ) { + const isExperimentalWebBinary = process + .env['EXPERIMENTAL_USE_WEB_BINARY_TEMPLATE']?.toLowerCase(); + if ( + isExperimentalWebBinary === 'false' + || isExperimentalWebBinary === '0' + ) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/webpack/template-webpack-plugin/src/WebEncodePlugin.ts` around lines 108 - 135, The opt-out check using process.env['EXPERIMENTAL_USE_WEB_BINARY_TEMPLATE'] is case-sensitive so values like "False" or "FALSE" will incorrectly enable the binary path; update the check around isExperimentalWebBinary in WebEncodePlugin.ts to perform a case-insensitive comparison (e.g. normalize via toLowerCase()) and guard for undefined/null so that only explicit "false" or "0" (in any case) choose the JSON branch; keep existing behavior for unset values to fall through to the binary path and reference the isExperimentalWebBinary variable and the surrounding return branches (JSON branch that uses genStyleInfo/tasmJSONInfo and the binary branch that imports encode).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In @.changeset/enable-web-binary-template-by-default.md:
- Line 2: The changeset currently marks "@lynx-js/template-webpack-plugin" as a
patch but the change flips default web template encoding (user-visible) so
update the changeset to use a minor bump instead of patch; in the .changeset
entry that contains the line "@lynx-js/template-webpack-plugin": patch change
"patch" to "minor" and ensure the changeset title/summary reflects the
feat(web): behavior change so the release tooling will produce a minor release.
- Line 18: The upgrade note sentence "**Upgrade to `@lynx-js/web-core@0.20.2`
could support the new output format**" is awkward; change that phrase to a
clearer instruction such as "**Upgrade to `@lynx-js/web-core@0.20.2` to support
the new output format**" or "**Upgrade to `@lynx-js/web-core@0.20.2` to enable
support for the new output format**" by editing the same line in the changeset.
---
Nitpick comments:
In `@packages/webpack/template-webpack-plugin/src/WebEncodePlugin.ts`:
- Around line 108-135: The opt-out check using
process.env['EXPERIMENTAL_USE_WEB_BINARY_TEMPLATE'] is case-sensitive so values
like "False" or "FALSE" will incorrectly enable the binary path; update the
check around isExperimentalWebBinary in WebEncodePlugin.ts to perform a
case-insensitive comparison (e.g. normalize via toLowerCase()) and guard for
undefined/null so that only explicit "false" or "0" (in any case) choose the
JSON branch; keep existing behavior for unset values to fall through to the
binary path and reference the isExperimentalWebBinary variable and the
surrounding return branches (JSON branch that uses genStyleInfo/tasmJSONInfo and
the binary branch that imports encode).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b3892b72-e2ef-44fb-8ef6-876e42badd79
📒 Files selected for processing (4)
.changeset/enable-web-binary-template-by-default.mdpackages/web-platform/web-core-e2e/scripts/generate-build-command.jspackages/web-platform/web-tests/package.jsonpackages/webpack/template-webpack-plugin/src/WebEncodePlugin.ts
💤 Files with no reviewable changes (1)
- packages/web-platform/web-core-e2e/scripts/generate-build-command.js
Signed-off-by: Haoyang Wang <12288479+PupilTong@users.noreply.github.com>
Signed-off-by: Haoyang Wang <12288479+PupilTong@users.noreply.github.com>
PupilTong
force-pushed
the
p/hw/opt-out-wasm
branch
from
93c37b9 to
8431c73
Compare
3 participants
Summary by CodeRabbit
New Features
Configuration
Compatibility
Chores / Tests
Documentation
Checklist