feat(compiler-core): resolve slot prop bindings as components#13573
feat(compiler-core): resolve slot prop bindings as components#13573edison1105 merged 11 commits intovuejs:minorfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request enhances the compiler's component resolution to support scoped slot bindings. When element tags reference identifiers from slot props, the compiler now resolves them directly rather than applying standard component resolution helpers, with fallback to normal resolution when identifiers are out of scope. Changes
Sequence Diagram(s)sequenceDiagram
participant Compiler
participant ResolveScopeRef as resolveScopeReference
participant Identifiers as context.identifiers
participant SetupResolution as Setup Binding<br/>Resolution
participant Fallback as RESOLVE_COMPONENT<br/>Fallback
participant CodeGen as Code Generation
Compiler->>ResolveScopeRef: resolveComponentType(tag)
ResolveScopeRef->>Identifiers: Check raw tag + variants<br/>(camelCase, PascalCase)
alt Found in Slot Scope
Identifiers-->>ResolveScopeRef: identifier match
ResolveScopeRef-->>CodeGen: Emit direct reference<br/>(_createVNode(slotProp.Foo))
else Not in Scope
Identifiers-->>ResolveScopeRef: no match
ResolveScopeRef->>SetupResolution: Check setup bindings
alt Setup Match
SetupResolution-->>CodeGen: Emit setup accessor
else No Setup Match
SetupResolution->>Fallback: Fallback to runtime resolution
Fallback-->>CodeGen: Emit _resolveComponent("Foo")
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes introduce new logic for identifier scope resolution with supporting test coverage across multiple file types, requiring understanding of component resolution mechanics and slot binding scoping rules. 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 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
❌ Deploy Preview for vue-sfc-playground failed. Why did it fail? →
|
❌ Deploy Preview for vue-next-template-explorer failed. Why did it fail? →
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/compiler-core/src/transforms/transformElement.ts (1)
388-391: Consider removing or documenting the commented-out code.The commented-out code for slot scope bindings handling should either be removed if not needed, or documented with a TODO comment explaining the future implementation plan.
- // const fromSlotScope = checkType(BindingTypes.SLOT_SCOPE) - // if (fromSlotScope) { - // return fromSlotScope - // } + // TODO: Future implementation for slot scope bindings + // const fromSlotScope = checkType(BindingTypes.SLOT_SCOPE) + // if (fromSlotScope) { + // return fromSlotScope + // }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/compiler-core/__tests__/transforms/transformElement.spec.ts(2 hunks)packages/compiler-core/src/codegen.ts(1 hunks)packages/compiler-core/src/options.ts(1 hunks)packages/compiler-core/src/transform.ts(2 hunks)packages/compiler-core/src/transforms/transformElement.ts(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/compiler-core/__tests__/transforms/transformElement.spec.ts (2)
packages/compiler-core/src/index.ts (1)
BindingTypes(11-11)packages/compiler-core/src/runtimeHelpers.ts (1)
RESOLVE_COMPONENT(26-28)
packages/compiler-core/src/codegen.ts (1)
packages/compiler-core/src/options.ts (1)
CodegenOptions(307-349)
🔇 Additional comments (7)
packages/compiler-core/src/options.ts (1)
203-203: LGTM! Well-integrated interface extension.The addition of the optional
identifiersproperty follows the existing pattern and maintains backward compatibility. The type signature is consistent with identifier tracking used elsewhere in the codebase.packages/compiler-core/src/codegen.ts (1)
123-126: LGTM! Proper type boundary maintenance.Adding
'identifiers'to the omitted properties list is correct and maintains type consistency. This follows the established pattern of excluding transform-specific properties from the codegen context.packages/compiler-core/src/transforms/transformElement.ts (2)
285-300: LGTM! Well-implemented slot prop component resolution.The new step correctly checks for components from slot props using
context.identifiers. The logic properly handles both direct tags and dot notation components, and is appropriately gated for non-browser builds.
302-302: LGTM! Proper step renumbering.The comment numbering has been correctly updated to reflect the new step insertion.
Also applies to: 319-319, 333-333
packages/compiler-core/src/transform.ts (2)
146-146: LGTM! Well-designed parameter addition.Adding the optional
identifiersparameter withObject.create(null)as the default is a good design choice. It enables external control of identifier tracking while maintaining backward compatibility.
191-191: LGTM! Proper context assignment.The
identifiersassignment correctly integrates the parameter into the transform context.packages/compiler-core/__tests__/transforms/transformElement.spec.ts (1)
124-141: Well-structured test for slot prop component resolution.The test correctly verifies that components referenced via scoped slot bindings (e.g.,
{Foo}) are not resolved as regular components. The test expectations are appropriate:
- No
RESOLVE_COMPONENThelper injection- Component not added to components list
- Root node tag remains as the parent component
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-sfc
@vue/compiler-ssr
@vue/compiler-vapor
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/runtime-vapor
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
|
Is this feature expected to be part of a minor release soon? |
Fuzzyma
force-pushed
the
allow-slot-props-components
branch
from
08e9cf2 to
dbfa866
Compare
|
Suggested final commit message: Didnt want to squash to retain history for now |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/compiler-core/__tests__/transforms/transformElement.spec.ts (1)
229-245: ⚡ Quick winAdd a namespaced out-of-scope fallback regression case.
You test in-scope
slotProps.Foo, but there isn’t a paired case proving that the same tag falls back to_resolveComponent(...)once outside the slot scope.Suggested test shape
+ test('does not resolve namespaced component from inactive scoped slot bindings', () => { + const { code } = baseCompile( + `<Example v-slot="slotProps"><slot-props.Foo /></Example><slot-props.Foo />`, + { + prefixIdentifiers: true, + isNativeTag: tag => tag !== 'slot-props.Foo', + bindingMetadata: { Example: BindingTypes.SETUP_CONST }, + }, + ) + + expect(code).toContain(`_createVNode(slotProps.Foo)`) + expect(code).toContain(`_resolveComponent("slot-props.Foo")`) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/compiler-core/__tests__/transforms/transformElement.spec.ts` around lines 229 - 245, Add a paired regression test to ensure namespaced tag "slot-props.Foo" falls back to component resolution outside slot scope: create a new test (e.g., "resolve namespaced component falls back out of scoped slot") that calls baseCompile on a template where <slot-props.Foo /> appears outside the v-slot (opposite of the existing "resolve namespaced component from scoped slot bindings" test) with the same compile options (prefixIdentifiers: true and isNativeTag returning false for 'slot-props.Foo'), then assert the generated code contains a call to _resolveComponent("slot-props.Foo") and/or uses _component_slot_props fallback and does not treat it as a scoped property (i.e., does not contain _createVNode(slotProps.Foo)); this will validate the out-of-scope fallback behavior.
🤖 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/transformElement.spec.ts`:
- Around line 229-245: Add a paired regression test to ensure namespaced tag
"slot-props.Foo" falls back to component resolution outside slot scope: create a
new test (e.g., "resolve namespaced component falls back out of scoped slot")
that calls baseCompile on a template where <slot-props.Foo /> appears outside
the v-slot (opposite of the existing "resolve namespaced component from scoped
slot bindings" test) with the same compile options (prefixIdentifiers: true and
isNativeTag returning false for 'slot-props.Foo'), then assert the generated
code contains a call to _resolveComponent("slot-props.Foo") and/or uses
_component_slot_props fallback and does not treat it as a scoped property (i.e.,
does not contain _createVNode(slotProps.Foo)); this will validate the
out-of-scope fallback behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e977eb7a-450c-470b-864e-d1e7c5949161
📒 Files selected for processing (3)
packages/compiler-core/__tests__/transforms/transformElement.spec.tspackages/compiler-core/src/transforms/transformElement.tspackages/compiler-ssr/__tests__/ssrComponent.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/compiler-core/src/transforms/transformElement.ts
Fuzzyma
changed the title
|
I think this currently does a bit more than “slot props”.
<template v-for="Foo in list">
<Foo />
</template>from resolving This should stay focused on slot props as components, so I think we need slot-scope-specific tracking instead of reading the generic |
|
@edison1105 I didnt even think about this case. But I also wonder why it shouldnt be allowed. It feels consistent when all identifiers can be used as components. do you see problems arising from this? otherwise I will look into it and see if I can keep slot identifier tracking seperate. |
|
The issue is not whether “all identifiers as components” feels consistent, but that For example: <template v-for="Foo in list">
<Foo :value="Foo" />
</template>Previously, the tag If users really want to render a component from a data variable, we already have the explicit form: <component :is="Foo" />The slot-prop case is different: the slot provider intentionally exposes a component constructor as part of the slot API, and using Also, since this is a feature rather than a bug fix, it should target the |
- remove identifiers from context (was only added to allow testing) - compile snippets in tests to test against compiled output - add ssr tests Co-authored-by: Copilot <copilot@github.com>
Properly differentiate between local identifiers (e.g. from `v-for`) and slot identifiers (e.g. from `v-slot`) during transformation. This ensures that components are only auto-resolved from scope variables when they originate specifically from slot props, preventing incorrect resolution when a component name is shadowed by a `v-for` variable. - Add `identifierScopes` to `TransformContext` to track scope types. - Update `addIdentifiers` and `vSlot` transform to mark slot-based identifiers. - Refactor `resolveComponentType` to use `isSlotScopeIdentifier`. - Ensure `identifierScopes` are correctly inherited in SSR sub-transforms. - Add comprehensive tests for shadowed and nested scope scenarios in both core and SSR compilers. Co-authored-by: Copilot <copilot@github.com>
Fuzzyma
force-pushed
the
allow-slot-props-components
branch
from
84d0691 to
97f408a
Compare
❌ Deploy Preview for vue-next-template-explorer failed. Why did it fail? →
|
|
@edison1105 I addressed the feedback by tracking the type of the identifier in an extra array and only allow resolution for the type "slot". Is this okay like this? // EDIT: the build fails because it tries to find the template-explorer in /packages but its in /packages-privat. I assume something is not correctly setup. Maybe in scripts/aliases.js? |
|
/ecosystem-ci run |
|
📝 Ran ecosystem CI: Open
|
4 participants
close #8553
I had a shot about implementing the issue I raised in #8553.
Happy for any feedback
Summary by CodeRabbit
New Features
kebab/camel-cased and namespaced usages. Slot scope bindings take
precedence over setup bindings during component resolution.
Tests