Dependency Injection Inheritance: Solution A #13924
Conversation
WalkthroughChild instances now get a prototypal Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App (root)
participant P as Parent
participant C as Child
participant Runtime as runtime-core
participant Provide as provide()
participant Inject as inject()
Note over Runtime: component instance creation
App->>Runtime: createComponentInstance(appContext)
Runtime->>P: create parent instance
Runtime->>C: create child instance
Runtime->>C: C.provides = Object.create(P.provides)
Note right of C #DFF0D8: prototype chain established at init
Note over Provide: providing a value
C->>Provide: provide(key, value)
Provide->>C: C.provides[key] = value
Note right of Provide #FFF3CD: no on-demand cloning/shadowing
Note over Inject: resolution
C->>Inject: inject(key)
Inject->>C: read C.provides[key] (falls back via prototype chain)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (3)
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 |
![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/runtime-core/src/component.ts (1)
636-636: Prototypalprovidescreation is correct; note perf trade‑offUsing
Object.create(parent.provides)(orappContext.provides) eagerly guarantees safe isolation and clean inheritance for DI. This is the right pairing with the updatedprovide().Trade‑off: every instance now allocates a
providesobject even if it never callsprovide(). Likely negligible, but worth a quick perf sanity check on large trees.If desired, I can help run or outline a micro-benchmark comparing before/after memory allocs and mount times on a large component graph.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/runtime-core/src/apiInject.ts(1 hunks)packages/runtime-core/src/component.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/runtime-core/src/apiInject.ts (1)
packages/runtime-core/src/component.ts (1)
currentInstance(711-711)
⏰ 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). (3)
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
🔇 Additional comments (1)
packages/runtime-core/src/apiInject.ts (1)
19-22: Aligns with upfront prototypal provides; avoids parent mutationWriting directly to
currentInstance.providesis correct now that each instance gets its ownprovideswith a prototype chain. This removes copy‑on‑write complexity and prevents accidental parent mutation.Please ensure tests cover:
- Child overriding a parent’s provide (descendants see child’s value).
- Root providing after app.provide (children see both via prototype).
- Symbol keys.
- Custom elements and nested createApp cases.
I can draft these tests if helpful.
Size ReportBundles
Usages
|
edison1105
added
the
🔨 p3-minor-bug
|
Could you please add a test? |
@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 I have now made the tests, I have tested the tests with the changes reverted (with it failing, like expected) and the the test passing with the changes in the PR (as expected). |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/runtime-core/__tests__/apiInject.spec.ts (4)
433-438: Add a tick after initial mount to avoid scheduler-order flakiness.Without awaiting a tick after the initial render, mounted hooks and the subsequent update triggered by shouldProvide can interleave, making the assertion timing-sensitive. Insert an extra await nextTick() right after render(h(Comp1), root).
Apply this diff:
const root = nodeOps.createElement('div') render(h(Comp1), root) + // ensure all mounted hooks have flushed before triggering the update + await nextTick() + shouldProvide.value = true await nextTick()
354-354: Use const for the ref.The variable isn’t reassigned; prefer const for clarity.
- let shouldProvide = ref(false) + const shouldProvide = ref(false)
357-371: Avoid shadowing by renaming the prop from "data" to "trigger".Each component declares a prop named data while also defining a local ref called data, which is confusing. Rename the prop to trigger and update call sites. This preserves the intent (force an update) and improves readability.
const Comp4 = { - props: ['data'], + props: ['trigger'], setup() { const data = ref('foo -1') @@ const Comp3 = { - props: ['data'], + props: ['trigger'], setup() { const data = ref('foo -1') @@ - return () => [ - h('div', data.value), - h(Comp4, { data: shouldProvide.value }), - ] + return () => [ + h('div', data.value), + h(Comp4, { trigger: shouldProvide.value }), + ] @@ - return () => [ - h('div', data.value), - h(Comp3, { data: shouldProvide.value }), - ] + return () => [ + h('div', data.value), + h(Comp3, { trigger: shouldProvide.value }), + ]Also applies to: 373-392, 386-389, 409-412
439-444: Polish the explanatory comment (optional).Minor typos; consider tightening wording for future readers.
- First (Root Component) should be "foo 0" because it is the Root Component and provdes shall only be injected to Descandents. - Second (Component 2) should be "foo 1" because it should inherit the provide from the Root Component - Third (Component 3) should be "foo 2" because it should inherit the provide from Component 2 (in the second render when shouldProvide = true) - Fourth (Component 4) should also be "foo 2" because it should inherit the provide from Component 3 which should inherit it from Component 2 (in the second render when shouldProvide = true) + First (root) should be "foo 0" (root cannot self-inject; falls back to default). + Second (Comp2) should be "foo 1" (inherits from root before updating its own provide). + Third (Comp3) should be "foo 2" (inherits Comp2’s updated provide during the second render). + Fourth (Comp4) should also be "foo 2" (inherits from Comp3, which inherits from Comp2).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/runtime-core/__tests__/apiInject.spec.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/runtime-core/__tests__/apiInject.spec.ts (2)
packages/runtime-core/src/apiInject.ts (2)
inject(36-74)provide(10-23)packages/runtime-test/src/serialize.ts (1)
serialize(10-20)
⏰ 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). (3)
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
🔇 Additional comments (1)
packages/runtime-core/__tests__/apiInject.spec.ts (1)
9-10: LGTM: lifecycle imports align with the test scenario.Importing onBeforeUpdate/onMounted is appropriate for validating DI updates across update cycles.
|
|
||
| provides: parent ? parent.provides : Object.create(appContext.provides), | ||
| provides: parent | ||
| ? Object.create(parent.provides) |
There was a problem hiding this comment.
The above comment should be added here
There was a problem hiding this comment.
The original comment was in apiInject.ts, but I can copy or move it to there too.
| } | ||
| const provides = currentInstance.provides | ||
| // TS doesn't allow symbol as index type | ||
| provides[key as string] = value |
There was a problem hiding this comment.
| provides[key as string] = value | |
| currentInstance.provides.provides[key as string] = value |
There was a problem hiding this comment.
I will integrate this into a commit, however provides const should be removed, and it would be currentInstance.provides not currentInstance.provides.provides
edison1105
added
wait changes
and removed
need test
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/runtime-core/src/apiInject.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/runtime-core/src/apiInject.ts (1)
packages/runtime-core/src/component.ts (1)
currentInstance(713-713)
⏰ 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). (3)
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
| // by default an instance it creates its own provides object | ||
| // using parent provides object as prototype. | ||
| // this way in `inject` we can simply look up injections from direct | ||
| // parent and let the prototype chain do the work. | ||
| const parentProvides = | ||
| currentInstance.parent && currentInstance.parent.provides | ||
| if (parentProvides === provides) { | ||
| provides = currentInstance.provides = Object.create(parentProvides) | ||
| } | ||
| // TS doesn't allow symbol as index type | ||
| provides[key as string] = value | ||
| currentInstance.provides[key as string] = value |
There was a problem hiding this comment.
Restore the per-instance provides fork before writing.
currentInstance.provides still aliases parent.provides until we fork it (see createComponentInstance in component.ts). With this change we now write straight into the parent’s map, so a child re-provide will bleed into siblings and ancestors. We need to restore the Object.create(parentProvides) guard (or otherwise ensure each instance owns its own map) before assigning the new value.
- // by default an instance it creates its own provides object
- // using parent provides object as prototype.
- // this way in `inject` we can simply look up injections from direct
- // parent and let the prototype chain do the work.
- // TS doesn't allow symbol as index type
- currentInstance.provides[key as string] = value
+ let { provides, parent } = currentInstance
+ if (parent) {
+ const parentProvides = parent.provides
+ if (provides === parentProvides) {
+ provides = currentInstance.provides = Object.create(parentProvides)
+ }
+ }
+ // TS doesn't allow symbol as index type
+ provides[key as string] = value📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // by default an instance it creates its own provides object | |
| // using parent provides object as prototype. | |
| // this way in `inject` we can simply look up injections from direct | |
| // parent and let the prototype chain do the work. | |
| const parentProvides = | |
| currentInstance.parent && currentInstance.parent.provides | |
| if (parentProvides === provides) { | |
| provides = currentInstance.provides = Object.create(parentProvides) | |
| } | |
| // TS doesn't allow symbol as index type | |
| provides[key as string] = value | |
| currentInstance.provides[key as string] = value | |
| let { provides, parent } = currentInstance | |
| if (parent) { | |
| const parentProvides = parent.provides | |
| if (provides === parentProvides) { | |
| provides = currentInstance.provides = Object.create(parentProvides) | |
| } | |
| } | |
| // TS doesn't allow symbol as index type | |
| provides[key as string] = value |
🤖 Prompt for AI Agents
In packages/runtime-core/src/apiInject.ts around lines 19 to 24, the code writes
directly into currentInstance.provides which still aliases parent.provides;
restore the per-instance provides fork before assigning so we don't mutate the
parent's map. Detect when currentInstance.provides === parent.provides (or when
it shares reference) and replace it with a shallow clone created from the parent
(e.g., Object.create(parentProvides)) so the instance owns its own map, then
assign the key (converting symbol keys to string as needed) into that
per-instance provides object.
|
I have now added your suggestions. |
close #13921
This is a solution to the issue #13921.
Summary by CodeRabbit
Bug Fixes
Refactor
Tests