dx(compiler-core): warn for :v-
#7359
Conversation
❌ Deploy Preview for vuejs-coverage failed.
|
skirtles-code
force-pushed
the
colon-before-directive
branch
from
02d0bd5 to
441e7a7
Compare
# Conflicts: # packages/compiler-dom/src/errors.ts # packages/compiler-ssr/src/errors.ts
Size ReportBundles
Usages
|
# Conflicts: # packages/compiler-core/src/parse.ts
CodSpeed Performance ReportMerging #7359 will not alter performanceComparing Summary
|
# Conflicts: # packages/compiler-core/__tests__/__snapshots__/parse.spec.ts.snap # packages/compiler-core/__tests__/parse.spec.ts # packages/compiler-core/src/errors.ts # packages/compiler-core/src/parse.ts # packages/compiler-dom/src/errors.ts # packages/compiler-ssr/src/errors.ts
edison1105
added
scope: compiler
🍰 p2-nice-to-have
WalkthroughA new warning detection mechanism is added to flag colon-prefixed directives with arguments starting with "v-", alongside corresponding error code enhancements and test infrastructure updates. Error codes are renumbered in DOM and SSR modules for alignment. Changes
Sequence DiagramsequenceDiagram
participant Parser
participant Directive Handler
participant Warning Handler
participant onWarn Callback
Parser->>Directive Handler: Parse directive argument
Directive Handler->>Directive Handler: Check if colon-prefixed<br/>and arg starts with 'v-'
alt Warning Condition Met
Directive Handler->>Warning Handler: emitWarning(X_COLON_BEFORE_DIRECTIVE)
Warning Handler->>onWarn Callback: Invoke with warning details
onWarn Callback->>onWarn Callback: Collect warning
end
Directive Handler->>Parser: Continue parsing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
@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/__tests__/parse.spec.ts (1)
2429-2439: Expand test coverage for common mistake patterns.The test correctly verifies the warning is emitted for
:v-foo, but consider adding tests for the most common real-world mistakes that motivated this PR:
:v-if,:v-for,:v-model,:v-show(common beginner mistakes mentioned in PR description)- Verify that
v-bind:v-foodoes NOT emit a warning (per PR description: "usingv-bind:v-is still allowed")- Test location accuracy (currently tests offset 5, which should be the colon position)
- Edge cases like
:v-with no directive nameApply this diff to add comprehensive test coverage:
X_COLON_BEFORE_DIRECTIVE: [ { code: `<div :v-foo="obj" />`, warnings: [ { type: ErrorCodes.X_COLON_BEFORE_DIRECTIVE, loc: { offset: 5, line: 1, column: 6 }, }, ], }, + { + code: `<div :v-if="show" />`, + warnings: [ + { + type: ErrorCodes.X_COLON_BEFORE_DIRECTIVE, + loc: { offset: 5, line: 1, column: 6 }, + }, + ], + }, + { + code: `<div :v-for="item in items" />`, + warnings: [ + { + type: ErrorCodes.X_COLON_BEFORE_DIRECTIVE, + loc: { offset: 5, line: 1, column: 6 }, + }, + ], + }, + { + code: `<div :v-model="value" />`, + warnings: [ + { + type: ErrorCodes.X_COLON_BEFORE_DIRECTIVE, + loc: { offset: 5, line: 1, column: 6 }, + }, + ], + }, + { + code: `<div v-bind:v-foo="obj" />`, + warnings: [], + }, ],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/compiler-core/__tests__/__snapshots__/parse.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
packages/compiler-core/__tests__/parse.spec.ts(2 hunks)packages/compiler-core/src/errors.ts(2 hunks)packages/compiler-core/src/parser.ts(2 hunks)packages/compiler-dom/src/errors.ts(1 hunks)packages/compiler-ssr/src/errors.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/compiler-core/src/errors.ts (1)
packages/runtime-core/src/index.ts (1)
ErrorCodes(141-141)
packages/compiler-core/src/parser.ts (1)
packages/compiler-core/src/ast.ts (1)
DirectiveNode(193-211)
packages/compiler-core/__tests__/parse.spec.ts (2)
packages/compiler-core/src/ast.ts (1)
Position(83-87)packages/compiler-core/src/parser.ts (1)
baseParse(1045-1096)
⏰ 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). (1)
- GitHub Check: test / e2e-test
🔇 Additional comments (7)
packages/compiler-dom/src/errors.ts (1)
24-24: LGTM! Mechanical renumbering to accommodate new core error code.The change from 53 to 54 aligns with the new
X_COLON_BEFORE_DIRECTIVEerror code added to the coreErrorCodesenum. The runtime check at line 42 ensures this remains in sync with the core extension point.packages/compiler-ssr/src/errors.ts (1)
20-20: LGTM! Cascading renumbering for SSR error codes.The change from 65 to 66 accounts for the DOM error code renumbering, maintaining proper alignment with
DOMErrorCodes.__EXTEND_POINT__. The runtime check at line 29 prevents collisions.packages/compiler-core/src/errors.ts (1)
93-93: LGTM! New error code properly integrated.The
X_COLON_BEFORE_DIRECTIVEerror code is well-positioned in the enum and has a clear message. The trailing colon in the message at line 183 correctly anticipates that the parser will append the specific attribute name when emitting the warning.Also applies to: 183-183
packages/compiler-core/src/parser.ts (2)
1029-1033: LGTM! Helper function follows existing pattern.The
emitWarningfunction correctly mirrors the structure ofemitErrorat line 1023, forwarding tocurrentOptions.onWarnwith a properly constructed compiler error.
262-272: Clarify the dynamic argument edge case—it is already handled correctly.The review's concern about dynamic arguments (point 1) is based on a misunderstanding. The code does catch cases like
:[v-model]:
- The parser extracts the argument from brackets via
arg.slice(1, -1), resulting inv-model(without brackets)- The check
arg.startsWith('v-')evaluates to true, and sincerawName === ':', the warning is correctly emittedThe other shorthand suggestions (
.v-and@v-) are not found in the codebase and fall outside the PR's scope—the warning intentionally targets only the:shorthand by checkingrawName === ':'.The message clarity suggestion (point 3) is valid but minor; rewording to
"':${arg}' is probably a mistake. Did you mean '${arg}' instead?"would improve readability if desired.Likely an incorrect or invalid review comment.
packages/compiler-core/__tests__/parse.spec.ts (2)
2558-2559: LGTM! Clean extension of test pattern structure.The addition of the
warnings?field alongsideerrors?maintains consistency with the existing pattern structure and allows tests to verify both error and warning behavior independently.
2444-2446: LGTM! Test infrastructure properly separates errors and warnings.The implementation correctly:
- Uses separate
errorSpyandwarnSpyinstances- Passes both to
baseParseviaonErrorandonWarnoptions- Destructures with default empty arrays for backward compatibility
- Asserts both errors and warnings independently
This allows existing tests to continue working while new tests can verify warning behavior.
Also applies to: 2453-2454, 2460-2461, 2470-2475
2 participants
This PR is closely related to my other PR, #7360. There is significant overlap between them and merging one will cause conflicts for the other.
This PR introduces a compiler warning when it encounters
:v-at the start of an attribute name. e.g.:v-if,:v-foror:v-model. This is almost invariably an error, and a common mistake made by beginners.Only the
:shorthand is affected. If someone really wants to have a DOM attribute beginning withv-then they can usev-bind:v-instead. Beginners are unlikely to make the mistake of usingv-bind:v-for, so I see no reason to prevent that.The compiler currently doesn't use warnings, only errors. However, I felt an error would be too severe for the initial introduction of this check as it would prevent some applications from building. It could be seen as a breaking change, as there's nothing in the docs to suggest that this isn't allowed. In future, once people have had chance to fix the warnings, we could maybe increase the severity to make it an error.
The warnings should be visible in the console for anyone using the SFC Playground or a browser-based compiler. For a standard Vite setup, the warning won't be shown in the browser, it'll only appear in the terminal running Vite.
SFC Playground example
Screenshot of Vite:
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.