refactor(compiler): add separate transform for vbind shorthand #13438
Conversation
WalkthroughAdded a new NodeTransform Changes
Sequence Diagram(s)sequenceDiagram
participant Parser
participant AST
participant transformVBindShorthand as ShorthandTransform
participant OtherTransforms
Parser->>AST: parse template → AST
AST->>ShorthandTransform: apply same-name v-bind expansion (first)
ShorthandTransform-->>AST: attach expression / report errors
AST->>OtherTransforms: continue with v-if, v-for, v-model, element transforms
OtherTransforms-->>AST: further node mutations (userKey, key resolution, helpers)
note right of ShorthandTransform #dff0d8: Shorthand handled before other transforms
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 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: |
edison1105
changed the title
![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)
19-19: Consider safer handling of directive argument access.The non-null assertion
prop.arg!assumes the argument will always exist for bind directives without expressions. While this may be valid for shorthand syntax, consider adding a guard check for robustness.- const arg = prop.arg! + const arg = prop.arg + if (!arg) continue
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/compiler-dom/__tests__/transforms/__snapshots__/vModel.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
packages/compiler-core/__tests__/transforms/vBind.spec.ts(2 hunks)packages/compiler-core/__tests__/transforms/vFor.spec.ts(2 hunks)packages/compiler-core/__tests__/transforms/vIf.spec.ts(3 hunks)packages/compiler-core/src/compile.ts(2 hunks)packages/compiler-core/src/index.ts(1 hunks)packages/compiler-core/src/transforms/transformVBindShorthand.ts(1 hunks)packages/compiler-core/src/transforms/vBind.ts(2 hunks)packages/compiler-core/src/transforms/vFor.ts(0 hunks)packages/compiler-dom/__tests__/transforms/vModel.spec.ts(3 hunks)packages/compiler-ssr/__tests__/ssrTransitionGroup.spec.ts(1 hunks)packages/compiler-ssr/src/index.ts(2 hunks)
💤 Files with no reviewable changes (1)
- packages/compiler-core/src/transforms/vFor.ts
🧰 Additional context used
🧬 Code Graph Analysis (6)
packages/compiler-core/src/compile.ts (2)
packages/compiler-core/src/index.ts (1)
transformVBindShorthand(69-69)packages/compiler-core/src/transforms/transformVBindShorthand.ts (1)
transformVBindShorthand(10-36)
packages/compiler-ssr/__tests__/ssrTransitionGroup.spec.ts (1)
packages/compiler-ssr/src/index.ts (1)
compile(32-94)
packages/compiler-ssr/src/index.ts (1)
packages/compiler-core/src/transforms/transformVBindShorthand.ts (1)
transformVBindShorthand(10-36)
packages/compiler-core/__tests__/transforms/vBind.spec.ts (1)
packages/compiler-core/src/transforms/transformVBindShorthand.ts (1)
transformVBindShorthand(10-36)
packages/compiler-core/src/transforms/vBind.ts (1)
packages/compiler-core/src/ast.ts (1)
createObjectProperty(673-683)
packages/compiler-core/__tests__/transforms/vFor.spec.ts (1)
packages/compiler-core/src/transforms/transformVBindShorthand.ts (1)
transformVBindShorthand(10-36)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
🔇 Additional comments (17)
packages/compiler-core/__tests__/transforms/vBind.spec.ts (1)
20-20: LGTM! Proper integration of transformVBindShorthand into test pipeline.The import and placement as the first transform in the test pipeline is correct, ensuring that shorthand v-bind syntax is properly expanded before other transforms process the AST.
Also applies to: 29-29
packages/compiler-core/src/index.ts (1)
69-69: LGTM! Proper public API exposure.Adding the export for
transformVBindShorthandcorrectly exposes the new transform as part of the compiler-core public API, consistent with other transform exports.packages/compiler-core/src/compile.ts (1)
25-25: LGTM! Correct integration into core compilation pipeline.The import and placement as the first transform in
getBaseTransformPresetis architecturally correct. This ensures shorthand v-bind directives are expanded before other transforms process them, maintaining the proper transformation order.Also applies to: 37-37
packages/compiler-ssr/src/index.ts (1)
16-16: LGTM! Consistent SSR pipeline integration.The import from
@vue/compiler-domand placement as the first transform in the SSR nodeTransforms array ensures consistent handling of shorthand v-bind syntax between client-side and server-side rendering pipelines.Also applies to: 59-59
packages/compiler-ssr/__tests__/ssrTransitionGroup.spec.ts (1)
104-125: LGTM! Well-structured test for SSR shorthand v-bind handling.The test correctly verifies that shorthand
:tagsyntax is properly transformed to use_ctx.tagin SSR compilation output. The inline snapshot expectation captures both the dynamic opening tag<${_ctx.tag}and closing tag</${_ctx.tag}>rendering, ensuring complete SSR functionality.packages/compiler-core/__tests__/transforms/vFor.spec.ts (2)
24-24: Import addition looks good.The
transformVBindShorthandimport is correctly added to support shorthand v-bind processing in v-for tests.
36-36: Correct transform order placement.Adding
transformVBindShorthandas the first node transform ensures that shorthand v-bind syntax (like:key) is expanded before the v-for transform processes it. This maintains proper dependency order in the transform pipeline.packages/compiler-dom/__tests__/transforms/vModel.spec.ts (3)
6-6: Import addition is correct.The
transformVBindShorthandimport enables shorthand v-bind processing in v-model tests.
22-22: Proper transform integration.Adding
transformVBindShorthandto the nodeTransforms ensures shorthand v-bind syntax is processed before element transformation, maintaining the correct pipeline order.
67-74: Excellent test case for edge case handling.This test addresses issue #13169 and correctly verifies that shorthand
:typebinding afterv-modeltriggers dynamic model behavior (V_MODEL_DYNAMIChelper). This is crucial because the input type affects which v-model transformation is applied, and shorthand syntax should be properly recognized.packages/compiler-core/__tests__/transforms/vIf.spec.ts (3)
20-25: Clean import restructuring.The import statement is properly restructured to include
transformVBindShorthandalong with other necessary imports.
43-48: Consistent transform pipeline setup.The addition of
transformVBindShorthandas the first node transform maintains consistency with other test suites and ensures proper processing order for shorthand v-bind syntax.
223-232: Thorough test for shorthand key binding.This test correctly verifies that shorthand
:keysyntax on v-if elements is properly processed. The expectation that botharg.contentandexp.contentequal 'key' confirms that the shorthand is expanded to:key="key"as intended. This addresses issue #11321 and ensures proper key handling in conditional rendering.packages/compiler-core/src/transforms/transformVBindShorthand.ts (3)
20-28: Error handling for invalid shorthand arguments is well-implemented.The validation correctly restricts shorthand syntax to static simple expressions only, and provides appropriate error reporting with fallback to an empty expression.
30-31: Camelization logic is correct for JavaScript property naming conventions.Converting the argument content to camelCase and creating a non-static expression properly handles the shorthand expansion (e.g.,
:foo-bar→:foo-bar="fooBar").
10-36: Well-structured transform that effectively modularizes shorthand v-bind handling.This dedicated transform cleanly separates the shorthand expansion logic from other bind-related transforms, improving maintainability and reducing complexity in other parts of the codebase.
packages/compiler-core/src/transforms/vBind.ts (1)
71-71: Non-null assertion on expression is safe in this context.The
exp!assertion is appropriate here since the earlier logic (lines 22-38) handles empty expressions, and at this pointexpshould be a valid expression node.
edison1105
changed the title
edison1105
added
scope: compiler
🔨 p3-minor-bug
edison1105
changed the title
edison1105
force-pushed
the
edison/refactor/vBindShorthand
branch
from
b68fcd1 to
503b4b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/compiler-core/src/transforms/transformVBindShorthand.ts (1)
32-38: Consider extracting validation logic to a helper function.The identifier validation logic (including the special hyphen case) could be extracted to improve readability and reusability, especially since it combines standard identifier validation with Vue-specific rules.
+function isValidPropIdentifier(name: string): boolean { + return name.length > 0 && ( + validFirstIdentCharRE.test(name[0]) || + // allow hyphen first char for https://github.com/vuejs/language-tools/pull/3424 + name[0] === '-' + ) +} + const propName = camelize((arg as SimpleExpressionNode).content) -if ( - validFirstIdentCharRE.test(propName[0]) || - // allow hyphen first char for https://github.com/vuejs/language-tools/pull/3424 - propName[0] === '-' -) { +if (isValidPropIdentifier(propName)) { prop.exp = createSimpleExpression(propName, false, arg.loc) }
📜 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)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/compiler-core/src/transforms/transformVBindShorthand.ts (3)
packages/compiler-core/src/index.ts (4)
transformVBindShorthand(69-69)NodeTransform(20-20)createCompilerError(34-34)ErrorCodes(32-32)packages/compiler-core/src/ast.ts (2)
createSimpleExpression(685-698)SimpleExpressionNode(225-247)packages/compiler-core/src/utils.ts (1)
validFirstIdentCharRE(77-77)
⏰ 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 / unit-test-windows
🔇 Additional comments (5)
packages/compiler-core/src/transforms/transformVBindShorthand.ts (5)
1-9: Clean and appropriate imports.The imports are well-organized and include all necessary dependencies for the transform implementation.
11-19: Correct node and directive filtering logic.The transform correctly identifies ELEMENT nodes and filters for bind directives without expressions, which is exactly what's needed for v-bind shorthand expansion.
20-29: Robust error handling for invalid arguments.The validation properly restricts shorthand syntax to static simple expressions and provides appropriate error reporting with fallback to an empty expression. This prevents runtime issues from malformed shorthand.
11-43: Well-architected transform implementation.The transform correctly implements the core logic for v-bind shorthand expansion:
- Processes only relevant ELEMENT nodes with bind directives
- Validates arguments appropriately
- Creates proper expression nodes with correct static/dynamic flags
- Handles edge cases with appropriate error reporting
This centralized approach should resolve the scattered handling issues mentioned in the PR objectives.
31-38: Keep the leading-hyphen special-case — it's intentional
Vue allows binding attribute names that start with '-' via dynamic arguments (v-bind:['-foo']); for CSS custom properties prefer :style or SFC v-bind(). This special-case aligns with Vue behavior — no change required.
edison1105
changed the title
|
/ecosystem-ci run |
|
📝 Ran ecosystem CI: Open
|
close #13169
close #13170
close #11321
close #12298
close #12828
use tests from #13170 and #12298 and #12828
The current handing logic for v-bind shorthand is rather scattered, which results in many edge cases where
transformBindShorthandneeds to be called additionally. these cases occur because vbind shorthand has not been processed when reading properties from props.This PR introduces a dedicated transform specifically for handling v-bind shorthand, and makes it the first transform to be called. This ensures that vbind shorthand has already been processed before any subsequent logic read from props
Special handling is required for double bind in vue-macros. PR vue-macros/vue-macros#960
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores