Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(runtime-core): fix inject with currentApp #11604

Closed
wants to merge 3 commits into from

Conversation

linzhe141
Copy link
Contributor

@linzhe141 linzhe141 commented Aug 13, 2024

const app = createApp({
  setup() {
    const childApp = createApp({
      setup() { },
    })
    expect(childApp.runWithContext(() => inject('bar'))).toBe('bar')
    return () => h('div')
  },
})

app.provide('bar', 'bar')

What is expected?
expect(childApp.runWithContext(() => inject('bar'))).toBe('bar')
What is actually happening?
image

Copy link

github-actions bot commented Aug 13, 2024

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 98.8 kB (+32 B) 37.4 kB (+12 B) 33.7 kB (+2 B)
vue.global.prod.js 156 kB (+32 B) 57.2 kB (+11 B) 50.8 kB (-18 B)

Usages

Name Size Gzip Brotli
createApp 54.2 kB (+32 B) 21 kB (+15 B) 19.1 kB (+18 B)
createSSRApp 58.1 kB (+32 B) 22.6 kB (+15 B) 20.6 kB (-4 B)
defineCustomElement 58.8 kB (+32 B) 22.5 kB (+11 B) 20.5 kB (+6 B)
overall 67.8 kB (+32 B) 26 kB (+14 B) 23.7 kB (+9 B)

@Gianthard-cyh
Copy link

Good job! But I think this won't fix the issue since it's not something really wrong with Vue itself. Please take a look at my comment here for more details: link.

@yyx990803 yyx990803 changed the base branch from main to 3.4 August 13, 2024 14:52
@yyx990803 yyx990803 changed the base branch from 3.4 to main August 13, 2024 14:53

if (currentApp) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should move this code above const provides = instance

@edison1105
Copy link
Member

edison1105 commented Aug 14, 2024

@Gianthard-cyh
this PR did not fix #11602.
It will fix another scene, but I don't know if this is needed.

Misunderstood.

@edison1105 edison1105 added the need guidance The approach/solution in the PR is unclear and requires guidance from maintainer to proceed further. label Aug 14, 2024
@Gianthard-cyh
Copy link

Gianthard-cyh commented Aug 14, 2024

@Gianthard-cyh this PR did not fix #11602. It will fix another scene, but I don't know if this is needed.

Yes, I think this PR is actually doing this: fall back to the component's provides if there is a currentApp but key is not found in its provides.

Also, this will affect the behavior of pinia when injecting a property provided by a component. The setup() function of pinia store is wrapped in runWithContext(), so currentApp is available. If the key is not found, it will fallback to the component's provides. If it is expected to receive an undefined (like we have discussed in #11602), this PR will break the behavior.

I think this needs to be further discussed: should inject() fall back to the component instance's provides when it is called in runWithContext() and key is not found? What is expected?

@linzhe141 linzhe141 closed this Aug 14, 2024
@linzhe141 linzhe141 reopened this Aug 14, 2024
@linzhe141
Copy link
Contributor Author

@Gianthard-cyh Thank you for your review, but I don’t know how to cancel the associated issue.

@yyx990803 yyx990803 closed this in 905c9f1 Aug 15, 2024
@yyx990803
Copy link
Member

It should not fallback because that would make the behavior of Pinia stores indeterministic.

yyx990803 added a commit that referenced this pull request Aug 15, 2024
@linzhe141 linzhe141 deleted the fix-inject branch September 27, 2024 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need guidance The approach/solution in the PR is unclear and requires guidance from maintainer to proceed further.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants