fix(compiler-sfc): demote const reactive bindings used in v-model #14214
Conversation
WalkthroughDisallows v-model on const bindings (emits new error) and demotes reactive const setup bindings targeted by v-model to let with a warning. Adds template analysis to collect v-model identifiers, updates compileScript flow, and includes tests and an enum alignment change. Changes
Sequence Diagram(s)sequenceDiagram
participant Template as Template Parser
participant Analysis as Template Analysis (importUsageCheck)
participant SFC as SFC Compiler (compileScript)
participant Bindings as Setup Bindings Metadata
participant vModel as v-model Transform
Template->>Analysis: parse template AST
Analysis->>SFC: resolveTemplateVModelIdentifiers() → vModelIds
SFC->>Bindings: check setup bindings for vModelIds
alt reactive setup const targeted by v-model
SFC->>SFC: mutate script AST (const → let)
SFC->>Bindings: update bindingMetadata (SETUP_LET)
SFC->>SFC: emit warnOnce about demotion
end
SFC->>vModel: run v-model transform with updated bindings
vModel->>Bindings: inspect binding type
alt literal const detected
vModel->>vModel: emit X_V_MODEL_ON_CONST error and abort
else writable binding
vModel->>vModel: generate assignment/event handler code
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)packages/compiler-sfc/src/script/importUsageCheck.ts (5)
⏰ 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). (2)
🔇 Additional comments (9)
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: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/compiler-sfc/__tests__/__snapshots__/compileScript.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
packages/compiler-core/__tests__/transforms/vModel.spec.ts(1 hunks)packages/compiler-core/src/errors.ts(2 hunks)packages/compiler-core/src/transforms/vModel.ts(1 hunks)packages/compiler-dom/src/errors.ts(1 hunks)packages/compiler-sfc/__tests__/compileScript.spec.ts(3 hunks)packages/compiler-sfc/src/compileScript.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/compiler-core/__tests__/transforms/vModel.spec.ts (1)
packages/compiler-core/src/index.ts (2)
BindingTypes(11-11)ErrorCodes(32-32)
packages/compiler-sfc/__tests__/compileScript.spec.ts (3)
packages/compiler-sfc/src/warn.ts (1)
warnOnce(3-10)packages/compiler-core/src/index.ts (1)
BindingTypes(11-11)packages/compiler-sfc/__tests__/utils.ts (1)
assertCode(27-42)
packages/compiler-sfc/src/compileScript.ts (3)
packages/compiler-sfc/src/index.ts (1)
SFCDescriptor(51-51)packages/compiler-core/src/ast.ts (1)
TemplateChildNode(93-102)packages/compiler-core/src/utils.ts (1)
isSimpleIdentifier(68-69)
⏰ 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). (2)
- GitHub Check: CodeQL analysis (javascript-typescript)
- GitHub Check: Agent
🔇 Additional comments (8)
packages/compiler-dom/src/errors.ts (1)
24-24: LGTM: Error code realignment.The numeric value update correctly shifts the DOM error codes to accommodate the new
X_V_MODEL_ON_CONSTerror added to the coreErrorCodesenum, preventing collision with the extension point.packages/compiler-sfc/src/compileScript.ts (1)
796-843: Verify demotion behavior is correctly scoped to reactive const bindings.The demotion logic specifically targets
SETUP_REACTIVE_CONSTbindings (line 806), which is the correct design since:
LITERAL_CONSTbindings (e.g.,const foo = 1) cannot be made writable and are properly rejected by the transform error atpackages/compiler-core/src/transforms/vModel.ts:52-58SETUP_REFbindings work with v-model via.valueassignment- Only
SETUP_REACTIVE_CONST(e.g.,const foo = reactive({})) can be "rescued" by demotion toletConfirm this aligns with the intended behavior and that the transform error provides clear guidance when literal const bindings are used with v-model.
packages/compiler-sfc/__tests__/compileScript.spec.ts (2)
1-18: LGTM: Proper test infrastructure for warning verification.The Vitest mock setup correctly intercepts warning calls, enabling the tests to verify that warnings are emitted during const binding demotion.
87-156: LGTM: Comprehensive test coverage for v-model const binding behavior.The tests properly verify:
- Reactive const bindings are demoted to
letwith appropriate warnings (both normal and inline template modes)- Literal const bindings correctly throw errors when used with v-model
- Binding metadata is updated to reflect the demotion (
SETUP_LET)This aligns with the intended behavior where only reactive const can be "rescued" by demotion, while literal const must error.
packages/compiler-core/src/errors.ts (2)
91-91: LGTM: New error code for const binding validation.The error code addition is properly positioned in the enum and integrates with the transform logic that validates v-model usage on constant bindings.
180-180: LGTM: Clear error message.The error message clearly explains the constraint and helps developers understand why v-model cannot be used on const bindings.
packages/compiler-core/__tests__/transforms/vModel.spec.ts (1)
586-601: LGTM: Test correctly verifies const binding rejection.The test properly validates that using v-model on a
LITERAL_CONSTbinding triggers theX_V_MODEL_ON_CONSTerror, following the same pattern as the existing props validation test.packages/compiler-core/src/transforms/vModel.ts (1)
51-58: LGTM: Proper validation for const bindings.The guard correctly rejects both
LITERAL_CONSTandSETUP_CONSTbindings from v-model usage, as these are not writable. This complements the SFC compiler's demotion logic forSETUP_REACTIVE_CONST, which can be converted tolet.
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where const reactive bindings (created via reactive()) cannot be properly updated when used with v-model. The fix implements a two-part solution: demoting const to let for reactive bindings in v-model, and adding compile-time validation to error on non-reactive const bindings.
Key changes:
- Demotes
constdeclarations of reactive bindings toletwhen they're used as v-model targets, enabling proper two-way binding - Adds compile-time error checking to prevent v-model usage on literal const and non-reactive const bindings
- Adds new error code
X_V_MODEL_ON_CONSTwith descriptive error message
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/compiler-sfc/src/compileScript.ts | Adds resolveTemplateVModelIdentifiers function to scan template AST for v-model usage, and implements logic to transform const to let for reactive bindings used in v-model |
| packages/compiler-sfc/tests/compileScript.spec.ts | Adds test mocking for warn functions and three test cases covering reactive binding demotion (both inline and non-inline modes) and error handling for literal const bindings |
| packages/compiler-sfc/tests/snapshots/compileScript.spec.ts.snap | Adds snapshots for the new test cases showing transformed output with let instead of const |
| packages/compiler-dom/src/errors.ts | Updates DOMErrorCodes enum start value from 53 to 54 to account for new error code added to core |
| packages/compiler-core/src/transforms/vModel.ts | Adds validation check to error when v-model is used on LITERAL_CONST or SETUP_CONST bindings |
| packages/compiler-core/src/errors.ts | Adds new error code X_V_MODEL_ON_CONST with error message explaining const bindings are not writable |
| packages/compiler-core/tests/transforms/vModel.spec.ts | Adds test case verifying error is thrown when v-model is used on const binding |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/ecosystem-ci run |
|
📝 Ran ecosystem CI: Open
|
edison1105
added
ready to merge
…ove template analysis caching
edison1105
force-pushed
the
edison/fix/11265
branch
from
4e1745b to
48d9ee5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/compiler-sfc/src/script/importUsageCheck.ts (1)
53-114: Template traversal misses v-model directives insidev-if/v-forstructures.The
walkfunction handlesNodeTypes.ELEMENTandNodeTypes.INTERPOLATION, butTemplateChildNodealso includesIfNodeandForNode. When an element hasv-iforv-for, Vue's parser wraps it in these structural nodes. Without handling them, v-model directives inside conditional/loop blocks are not detected.For example,
<input v-if="show" v-model="foo">creates an IfNode, and the v-model on the input won't be collected.Apply this diff to handle structural node types:
case NodeTypes.INTERPOLATION: extractIdentifiers(ids, node.content) break + case NodeTypes.IF: + node.branches.forEach(branch => branch.children.forEach(walk)) + break + case NodeTypes.FOR: + node.children.forEach(walk) + break } }You'll also need to import
NodeTypesvalues for IF and FOR if not already available.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/compiler-sfc/src/compileScript.ts(2 hunks)packages/compiler-sfc/src/script/importUsageCheck.ts(4 hunks)
🔇 Additional comments (5)
packages/compiler-sfc/src/script/importUsageCheck.ts (3)
23-36: Clean cache unification.The refactored cache structure and the two accessor functions (
resolveTemplateVModelIdentifiers,resolveTemplateUsedIdentifiers) provide a clean separation of concerns while avoiding duplicate template traversals.
38-49: LGTM!The unified analysis function correctly caches and returns both identifier sets.
72-84: Correct filtering for simple identifiers.The logic appropriately captures only simple identifier expressions for v-model targets, excluding complex expressions like member access which wouldn't need declaration demotion.
packages/compiler-sfc/src/compileScript.ts (2)
67-70: LGTM!Clean import addition for the new helper function.
766-813: Well-structured demotion logic with correct placement.The fix correctly addresses the v-model const binding issue. A few observations:
- The guard condition properly ensures template analysis is only performed when a valid template AST exists.
- When a
constdeclaration has multiple declarators (e.g.,const foo = reactive({}), bar = ref(0)), the entire declaration becomeslet, but only the demoted identifier's binding metadata is updated. This is acceptable since Vue uses binding metadata (not the source keyword) for setter generation.- The warning message clearly explains the transformation to developers.
close #11265
close #11275
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.