fix(compiler-core): fix v-bind shorthand handling for in-DOM templates #13933
+27
−2
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughAdds a browser-only branch treating a present Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant BrowserDOM as In‑DOM template (browser)
participant Parser as baseParse
participant Transform as transformVBindShorthand
participant Reporter as Diagnostic Reporter
BrowserDOM->>Parser: parse attributes (shorthand v-bind)
Parser->>Transform: directive node (argument + expression)
alt missing expression OR (SIMPLE_EXPRESSION && empty content && __BROWSER__ && !__TEST__)
Transform->>Reporter: emit missing/invalid-argument diagnostic
Reporter-->>Transform: diagnostic recorded
Transform-->>Parser: treat as no-expression shorthand (error path)
else valid expression
Transform-->>Parser: normal shorthand transform -> ObjectExpression properties
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-sfc
@vue/compiler-ssr
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/compiler-core/src/transforms/transformVBindShorthand.ts (1)
18-20: Also treat whitespace-only values as emptyIn some in-DOM parses, attribute values can be whitespace-only. Trim before checking emptiness to make the fix more robust.
Apply this diff:
- (!prop.exp || - (prop.exp.type === NodeTypes.SIMPLE_EXPRESSION && !prop.exp.content)) + (!prop.exp || + (prop.exp.type === NodeTypes.SIMPLE_EXPRESSION && + prop.exp.content.trim().length === 0))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/compiler-core/src/transforms/transformVBindShorthand.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
🔇 Additional comments (1)
packages/compiler-core/src/transforms/transformVBindShorthand.ts (1)
18-20: Good fix: handles empty SIMPLE_EXPRESSION to avoid in-DOM crashExtending the condition to treat an empty simple expression as absent prevents the TypeError in in-DOM templates. Short‑circuiting keeps
.typeaccess safe.Please add a regression test for in-DOM templates covering:
to ensure both parse to same-name shorthand without crashing.
edison1105
force-pushed
the
edison/fix/13930
branch
from
1d35971 to
4685dac
Compare
edison1105
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/compiler-core/__tests__/transforms/vBind.spec.ts (1)
115-134: Restore the previous__BROWSER__flag valueBecause
__BROWSER__is a shared global across the suite, forcing it back tofalsecould break any test that intentionally left ittrue. Cache the old value and restore it infinallyto keep the suite isolated.- try { - __BROWSER__ = true + const prevBrowser = __BROWSER__ + try { + __BROWSER__ = true // in-DOM templates will be parsed by the browser into :id="" const node = parseWithVBind(`<div :id="" />`) const props = (node.codegenNode as VNodeCall).props as ObjectExpression expect(props.properties[0]).toMatchObject({ key: { content: `id`, isStatic: true, }, value: { content: `id`, isStatic: false, }, }) } finally { - __BROWSER__ = false + __BROWSER__ = prevBrowser }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/compiler-core/__tests__/transforms/vBind.spec.ts(1 hunks)packages/compiler-core/src/parser.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/compiler-core/__tests__/transforms/vBind.spec.ts (1)
packages/compiler-core/src/ast.ts (2)
VNodeCall(323-341)ObjectExpression(374-377)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
🔇 Additional comments (1)
packages/compiler-core/src/parser.ts (1)
1057-1060: Decode-entities guard still enforces runtime expectationsThanks for tightening the condition to skip only under
__TEST__; this keeps real browser builds protected while letting tests flip__BROWSER__without tripping the check. Looks good.
edison1105
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/compiler-core/__tests__/transforms/vBind.spec.ts (1)
115-134: Restore__BROWSER__to its original valueWe shouldn’t assume the ambient
__BROWSER__flag starts asfalse. If another spec toggles it totrue, this test will force it back tofalseand leak state across suites. Capture the current value and restore it in thefinallyblock to keep the test isolated.- test('no expression (shorthand) in-DOM templates', () => { - try { - __BROWSER__ = true + test('no expression (shorthand) in-DOM templates', () => { + const prevBrowser = __BROWSER__ + try { + __BROWSER__ = true @@ - } finally { - __BROWSER__ = false + } finally { + __BROWSER__ = prevBrowser } })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/compiler-core/__tests__/transforms/vBind.spec.ts(1 hunks)packages/compiler-core/src/transforms/transformVBindShorthand.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/compiler-core/__tests__/transforms/vBind.spec.ts (1)
packages/compiler-core/src/ast.ts (2)
VNodeCall(323-341)ObjectExpression(374-377)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
edison1105
added
ready to merge
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
close #13930
Summary by CodeRabbit
Bug Fixes
Tests