Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9k
fix(compiler-sfc): demote const reactive bindings used in v-model#14214
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
coderabbitaibot commentedDec 17, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 endEstimated 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
|
pkg-pr-newbot commentedDec 17, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@vue/compiler-core@vue/compiler-dom@vue/compiler-sfc@vue/compiler-ssr@vue/reactivity@vue/runtime-core@vue/runtime-dom@vue/server-renderer@vue/sharedvue@vue/compatcommit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull request overview
This PR fixes an issue whereconst reactive bindings (created viareactive()) cannot be properly updated when used withv-model. The fix implements a two-part solution: demotingconst tolet 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 | AddsresolveTemplateVModelIdentifiers function to scan template AST for v-model usage, and implements logic to transformconst tolet 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 withlet instead ofconst |
| 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 codeX_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.
edison1105 commentedDec 17, 2025
/ecosystem-ci run |
vue-bot commentedDec 17, 2025
📝 Ran ecosystem CI:Open
|
…ove template analysis caching
4e1745b to48d9ee5CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Uh oh!
There was an error while loading.Please reload this page.
close#11265
close#11275
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.