feat(compiler): emit lazy getter for scoped slot props#14494
Open
Mini-ghost wants to merge 1 commit intovuejs:mainfrom
Open
feat(compiler): emit lazy getter for scoped slot props#14494Mini-ghost wants to merge 1 commit intovuejs:mainfrom
Mini-ghost wants to merge 1 commit intovuejs:mainfrom
Conversation
📝 WalkthroughWalkthroughThis pull request introduces support for Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
🧹 Nitpick comments (3)
packages/compiler-core/__tests__/transforms/transformSlotOutlet.spec.ts (1)
519-522: Minor inconsistency: UsetoBeUndefined()for consistency.Line 521 uses
.toBe(undefined)while other similar assertions in this file use.toBeUndefined().🔧 Consistency fix
- expect(props.properties[0].isGetter).toBe(undefined) + expect(props.properties[0].isGetter).toBeUndefined()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/compiler-core/__tests__/transforms/transformSlotOutlet.spec.ts` around lines 519 - 522, Replace the inconsistent Jest assertion in the test that checks the slot outlet props: change the assertion on props.properties[0].isGetter from expect(...).toBe(undefined) to expect(...).toBeUndefined() so it matches the other assertions in transformSlotOutlet.spec.ts; locate the assertion using the props variable (derived from (ast.children[0] as ElementNode).codegenNode.arguments[2]) and update the expect call accordingly.packages/compiler-core/src/transforms/transformSlotOutlet.ts (1)
142-171: Object identity check may fail for transformed expressions in edge cases.The lazy evaluation marking relies on
lazyEvalExps.has(prop.value)which uses object identity. While this works for simple identifiers (filtered at line 119), ifbuildPropstransforms the expression node (e.g., viaprocessExpressioncreating a new compound expression), the identity check will fail silently.The current implementation is protected by the
isSimpleIdentifierfilter, but consider adding a comment explaining this constraint for future maintainers.📝 Suggested documentation
if (lazyEvalExps.size > 0 && slotProps) { + // Note: lazyEvalExps contains original expression node references. + // This identity check works because we only track simple identifiers + // (see isSimpleIdentifier filter above), which are not transformed + // into new compound expression nodes by buildProps. const propsObjects: ObjectExpression[] = []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/compiler-core/src/transforms/transformSlotOutlet.ts` around lines 142 - 171, Add a short inline comment above the block that checks lazyEvalExps against prop.value in transformSlotOutlet.ts explaining that lazyEvalExps uses object identity (lazyEvalExps.has(prop.value)) and thus depends on nodes not being replaced by transformations (e.g., buildProps/processExpression may create new compound expression nodes), and note that this is why the earlier isSimpleIdentifier filter is required; also advise future maintainers to ensure any transformation preserves the original node identity or to update this logic to match transformed nodes if they change the AST flow.packages/compiler-core/__tests__/transforms/vModel.spec.ts (1)
616-636: Consider more precise assertion for generated handler structure.The test uses loose
expect.stringContainingmatchers which could miss subtle regressions. Consider matching the full structure similar to existing tests in this file.🔧 More precise assertion
- expect(props[1]).toMatchObject({ - key: { content: 'onUpdate:modelValue', isStatic: true }, - value: { - children: expect.arrayContaining([ - expect.stringContaining('=> (('), - expect.stringContaining(').value = $event)'), - ]), - }, - }) + expect(props[1]).toMatchObject({ + key: { content: 'onUpdate:modelValue', isStatic: true }, + value: { + children: [ + '$event => ((', + { content: 'title.value' }, + ') = $event)', + ], + }, + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/compiler-core/__tests__/transforms/vModel.spec.ts` around lines 616 - 636, Update the test "generates .value assignment in event handler" to assert the handler shape more precisely instead of using loose expect.stringContaining checks: locate the parseWithVModel call and the computed node extraction (root -> node -> (node.codegenNode as VNodeCall).props as ObjectExpression).properties, then replace the two stringContaining matchers with a stricter match that mirrors existing tests (e.g., assert the exact generated handler source string or full children array structure for props[1].value.children) so the assertion verifies the exact arrow function wrapper and the final ").value = $event" fragment rather than any substring; keep references to the same symbols (parseWithVModel, VNodeCall, props) when updating the expectation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/compiler-core/__tests__/transforms/transformSlotOutlet.spec.ts`:
- Around line 519-522: Replace the inconsistent Jest assertion in the test that
checks the slot outlet props: change the assertion on
props.properties[0].isGetter from expect(...).toBe(undefined) to
expect(...).toBeUndefined() so it matches the other assertions in
transformSlotOutlet.spec.ts; locate the assertion using the props variable
(derived from (ast.children[0] as ElementNode).codegenNode.arguments[2]) and
update the expect call accordingly.
In `@packages/compiler-core/__tests__/transforms/vModel.spec.ts`:
- Around line 616-636: Update the test "generates .value assignment in event
handler" to assert the handler shape more precisely instead of using loose
expect.stringContaining checks: locate the parseWithVModel call and the computed
node extraction (root -> node -> (node.codegenNode as VNodeCall).props as
ObjectExpression).properties, then replace the two stringContaining matchers
with a stricter match that mirrors existing tests (e.g., assert the exact
generated handler source string or full children array structure for
props[1].value.children) so the assertion verifies the exact arrow function
wrapper and the final ").value = $event" fragment rather than any substring;
keep references to the same symbols (parseWithVModel, VNodeCall, props) when
updating the expectation.
In `@packages/compiler-core/src/transforms/transformSlotOutlet.ts`:
- Around line 142-171: Add a short inline comment above the block that checks
lazyEvalExps against prop.value in transformSlotOutlet.ts explaining that
lazyEvalExps uses object identity (lazyEvalExps.has(prop.value)) and thus
depends on nodes not being replaced by transformations (e.g.,
buildProps/processExpression may create new compound expression nodes), and note
that this is why the earlier isSimpleIdentifier filter is required; also advise
future maintainers to ensure any transformation preserves the original node
identity or to update this logic to match transformed nodes if they change the
AST flow.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/compiler-core/__tests__/transforms/__snapshots__/transformExpressions.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (15)
packages-private/template-explorer/src/options.tspackages/compiler-core/__tests__/transforms/transformElement.spec.tspackages/compiler-core/__tests__/transforms/transformExpressions.spec.tspackages/compiler-core/__tests__/transforms/transformSlotOutlet.spec.tspackages/compiler-core/__tests__/transforms/vModel.spec.tspackages/compiler-core/src/ast.tspackages/compiler-core/src/codegen.tspackages/compiler-core/src/options.tspackages/compiler-core/src/transforms/transformElement.tspackages/compiler-core/src/transforms/transformExpression.tspackages/compiler-core/src/transforms/transformSlotOutlet.tspackages/compiler-core/src/transforms/vModel.tspackages/compiler-sfc/__tests__/compileScript.spec.tspackages/compiler-sfc/__tests__/compileScript/defineProps.spec.tspackages/compiler-sfc/src/compileScript.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Close #4192
Summary by CodeRabbit
New Features
Tests