refactor(compiler): add dedicated transform for vbind shorthand #13438
Conversation
WalkthroughA new node transform, Changes
Sequence Diagram(s)sequenceDiagram
participant Parser
participant AST
participant transformVBindShorthand
participant OtherTransforms
Parser->>AST: Parse template to AST
AST->>transformVBindShorthand: Apply shorthand v-bind expansion
transformVBindShorthand->>AST: Update nodes with expanded expressions
AST->>OtherTransforms: Continue with other node transforms (e.g., vIf, vFor, vModel)
OtherTransforms->>AST: Further transform AST
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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
|
/ecosystem-ci run |
edison1105
added
scope: compiler
🔨 p3-minor-bug
|
📝 Ran ecosystem CI: Open
|
edison1105
changed the title
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