vuejs / core

🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
https://vuejs.org/
MIT License
47.89k stars 8.36k forks source link

template ref can not get exposed value from a top level await component #4930

Open edison1105 opened 3 years ago

edison1105 commented 3 years ago

Version

3.2.21

Reproduction link

sfc.vuejs.org/

Steps to reproduce

click the button

What is expected?

without error

What is actually happening?

got an error

edison1105 commented 3 years ago

see https://github.com/vuejs/vue-next/issues/4891#issuecomment-964762807 the component is not resolved when setRef. image

edison1105 commented 3 years ago

as a workaround, from @sodatea defineExpose should run before the first await. see demo

languanghao commented 3 years ago

@edison1105 I tried that, but I do not think it's a good workaround.

As code below, expose method need invoke a var returned by a async method.

const props = defineProps<id: string>()

const recorder = await getRecorder(props.id)
const save = () => {
    // post recorder to server
}
defineExpose({ record })

If I move defineExpose({ record }) to the front of async function, then I have to change recorder from const to var.

这种写法存在代码上的逻辑撕裂,如果可能的话请加以增强。

LinusBorg commented 3 years ago

Currently, for template refs Vue relies on the presence of instance.exposed to decide wether this is a closed instance – requiring an exposeProxy– or a normal, open instance which exposes the normal instance.proxy.

This is usually fine, but becomes a problem with an async component like the one in this issue, because here, instance.exposed will be set after the instance was assigned to the parent's template ref, so the parent''s ref doesn't point to the exposeProxy.

So what we would need is a way to recognize that a component was created with script setup and is therefore closed by default right when the component instance is being created, not only once exposed() has been called, maybe a option flag we sneak in during compilation.

But that would still leave us with a challenge: While the async component is still pending, instance.exposeProxy would exist, but still be unaware of the sayHi function as expose() hasn't been called yet. That would mean that comp.sayHi would still be unavailable until the async component has resolved. The example of OP would still fail, in my understanding.

So in summary:

So fix or not, the general recommendation should be to call expose() before any top level await calls in setup.

languanghao commented 3 years ago

@LinusBorg Thank you for your reply.

Sometimes call expose() before any top level await calls will make code logic strange as I replied before. The exposed fuction invkoe var which defined in setup is very normal.

Consider exposed function need invoke a var which created by an async function, then we only have tow choises:

  1. define the var after the exposed function
    const save = () => {
    // where is the declare code for recorder?
    console.log(recorder)
    }
    const recorder = await getRecorder()
  2. declare the var from const to let whithout init value before the exposed function
    // delcare the recorder without init, so we need special the type, and we need change const to let
    let recorder: RecorderType
    const save = () => {
    console.log(recorder)
    }
    recorder = await getRecorder()

The first option violates the logic of writing code, we always invoke a var after we declared it. Here we change the code order, only because we need let the defineExpose work as expected.

The second option seems tobe a good choise than the first one, but still make the code reader confusion why we need split declare and init, where is the init code.

So I think we should find a way to improve the defineExposed macro to work better.

jods4 commented 2 months ago

@LinusBorg It would be nice if something was done to make defineExpose usable after await. You can see this issue was referenced many times. It's sometimes quite inconvenient to pass a strongly typed object to defineExpose when all its constituent won't be declared until later.

Your idea of having the compiler set a hasExpose flag on component sounds ok. Or an alternative could be for the compiler to define instance.exposed to a temporary value in async component so that it exists and the ref is considered an exposed component, even if it will be re-defined later.

But that would still leave us with a challenge: While the async component is still pending, instance.exposeProxy would exist, but still be unaware of the sayHi function as expose() hasn't been called yet. That would mean that comp.sayHi would still be unavailable until the async component has resolved. The example of OP would still fail, in my understanding.

I don't think that's what happens. The template ref will remain undefined until the async component resolves and is mounted in DOM. At this point, all the setup code has executed and the exposed object should be "complete". You can observe that it seems to work in this Vue Playground.

Another argument is that this is actually what people have to do today. The work-around is to pass an incomplete object to defineExpose and fill it in after await. We do that, as we have to. If that incomplete exposed object was visible in template refs before async resolves, we would have these same issues, too. Thankfully it seems to not be the case.

If I'm correct whether you can call defineExpose before or after await makes no difference in real-world usage, but it's a great improvement to DX, especially with TS typing. If it's just a matter of the compiler defining instance.exposed at the beginning of async components that contain defineExpose macro, I think Vue should do it.

jods4 commented 2 months ago

BTW, if Vue does not improve defineExpose to be usable after await, this limitation should be a warning in the docs of defineExpose.

edison1105 commented 2 months ago

@jods4 I've made a PR to fix this one. but I am not even sure it is the proper fix. Please feel free to review.

jods4 commented 2 months ago

@edison1105 To be honest it's not what I expected, based on @LinusBorg comment above.

My main worry with your queuePostRenderEffect approach is timing. I'm afraid the exposed value would have a different timing than what's observable in sync components. Your test waits until the child component is fully resolved, and Vue queue has been processed. Devs usually expect that refs are set when onMounted is called, and I don't think it would be the case with your PR (to be fair: I have not run the code to verify).

The approach I assumed from Linus' description was this:

  1. The compiler is modified so that when an async component contains a defineExpose macro, then instance.exposed = {} is introduced at the beginning of setup() so that next step sees a defined exposed member. This will be later replaced by the right object when the user defineExpose is called (after awaiting).
  2. This code in getComponentPublicInstance is modified: https://github.com/vuejs/core/blob/main/packages/runtime-core/src/component.ts#L1179-L1194. Because of 1. it will see an exposed property and create the proxy. What remains to be done is being able to swap the instance when the component later calls defineExpose asynchronously. One cannot change the target of a Proxy, but I think there might be an opportunity to simplify code here. The proxy could wrap instance directly and perform similarly -- and support changing the exposed object. This might be a start?
    instance.exposeProxy = new Proxy(instance, {
    get(target, key: string) {
      if (key in target.exposed) {
        return unref(target.exposed[key])
      } else if (key in publicPropertiesMap) {
        return publicPropertiesMap[key](instance)
      }
    },
    has(target, key: string) {
      return key in target.exposed || key in publicPropertiesMap
    },
    }))

It's worth noting that the proxy handler is identical for all instances, so it could be cached and reused for all components (saving a bit of memory).

edison1105 commented 1 month ago

@jods4 We cannot fundamentally solve this problem, for example, with the following usage:

<script lang="ts" setup>
const data = await fetch(...)

const getFoo = () => {
    return data.foo
}
defineExpose({
    getFoo,
})
</script>

When expose relies on the result of an asynchronous request. Although this approach may be unreasonable, it is allowed.

jods4 commented 1 month ago

@edison1105 This example is exactly the improvement that this issue would like to perform. If expose does not rely on async results, then it can trivially be put at the top script setup.

What is the fundamental problem that can't be solved in this example?

edison1105 commented 1 month ago

Scenario 1:

If expose does not rely on asynchronous requests, it needs to be placed before the async call to solve the issue. For defineExpose, during compilation, we can easily move __expose above the async request. However, if users manually call expose without using defineExpose:

export default {
  async setup(_, { expose }) {
    await fetch('...')
    expose({
      data: 'foo'
    })
  }
}

We cannot handle this automatically. We must inform users through documentation to manually place expose above the async request.

Scenario 2:

If expose relies on the result of an asynchronous request, like this:

<script lang="ts" setup>
const data = await fetch(...)
const getFoo = () => {
  return data.foo
}
defineExpose({
  getFoo,
})
</script>

or

export default {
  async setup(_, { expose }) {
    const data = await fetch(...)
    const getFoo = () => {
      return data.foo
    }
    expose({ getFoo })
  }
}

Before the async request completes (before the component mounts), if the parent component calls the child component's getFoo via ref in mounted, it won't get the data (currently because instance.exposed = null causing an incorrect refValue). Even if we modify it as mentioned above (making instance.exposed = {} during compilation and reassigning instance.exposed after the async request completes), we still need to inform users that they must wait until the async request completes (until the child component finishes rendering) to get the correct result. We cannot fundamentally solve the issue of making users wait.

Conclusion:

Considering the above two scenarios, regardless of whether expose relies on an async request, the behavior should be consistent: functions or data exposed by expose should not be callable before the async component mounts. In other words, setRef should only be done after the async component renders. This is the approach of PR #12082. see Playground with #12082

jods4 commented 1 month ago

@edison1105

Before the async request completes (before the component mounts), if the parent component calls the child component's getFoo via ref in mounted

This is the culprit. My understanding is that an async component does not mount before its async setup completes. So the exposed component is not visible to any parent before it takes its final value. This would mean that this is not an issue and the solution proposed above would work. Your sentence actually contradicts itself as you say both "before the component mounts" and "calls the child in mounted".

This playground I shared previously seems to confirm that it's a working solution.

Adding properties to the exposed objects is also the work-around that we currently employ and I have not seen an issue with it so far.