search

vuejs / core

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

Ref (DOM element) should work with non-ref (reactivity) #660

Closed jods4 closed 4 years ago

jods4 commented 4 years ago

What problem does this feature solve?

The way it currently (doesn't) works is really surprising for users. It took me a long amount of time to understand why I couldn't get a DOM element from my code.

Here's a simplified repro:

<button ref=b1></button>
<button ref=b2></button>
const MyApp = {
  setup() {
    return {
      b1: ref(null),
      b2: null,
    };
  }
};

Looking at this code, my intuition is that b1 will be a ref containing the first HTMLButton and b2 will be a non-reactive field containing the second HTMLButton.

With vue-next@3.0.0-alpha.3, b1 works as expected but b2 stays null. This is confusing, especially since in templating the ref nature of b1 is hidden -- code using b1 and b2 is the same in template expressions.

Moreover for better usability you may be tempted to hide all your refs behind a getter/setter, I'm doing this:

const MyApp {
  // hideRefs is a function that converts every field containing a ref to a getter/setter wrapping that ref,
  // making it very easy and consistent to use state inside and outside templates.
  // So b1 becomes 
  //   let r = ref(null)
  //   get b1() { return r.value }
  //   set b1(v) { r.value = v }
  let state = hideRefs({
    b1: ref(null),
    b2: null,
  });
  return state;
}

And now even though b1 is reactive, it's null just like b2 because Vue doesn't see the ref hidden inside the getter/setter.

Similar issues arise when using the built-in reactive({ b1: null, b2: null}).

What does the proposed API look like?

There is no new API surface.

I propose to change the behavior of <div ref=x> so that the HTML element is available in setup state whether the field is a ref or not.

If there is a technical reason to not do that, I suggest that you detect this and provide a warning in DEV mode.

posva commented 4 years ago

Why would you want a more implicit behavior without saying it's a ref? Why is this more confusing to you? Is it because before it was that implicit as well: it was only necessary to declare ref="a" in the template and that gave you access to the Component/Element reference?

jods4 commented 4 years ago

@posva Do you find the behavior of the first repro above intuitive for a newcomer?

I ended up in that situation (more involved, but basically this root cause) and I lost a lot of time figuring out why I didn't get the element. It was not a nice experience.

Moreover I didn't try but I expect this to not work either:

setup() {
  let scope = reactive({ b1: null })
  return scope
}

Because there is no ref involved, just a proxy. This pattern is found on the Vue Composition API cheatsheet.

It's also inconsistent with other stuff that updates the scope, like v-model. Why would v-model write to any property; but ref doesn't?

This is bad developer experience. What are the arguments against making it work with any property?

If you don't fix it, you should document this more clearly, with a red box in the docs and create a warning in DEV mode if you detect this situation (property exists but is not a ref).

posva commented 4 years ago

@jods4

If you don't fix it, you should document this more clearly, with a red box in the docs and create a warning in DEV mode if you detect this situation (property exists but is not a ref).

I'm asking you the questions to get a better understanding, never said this was or wasn't going to be fixed 🙂 .

To answer your questions:

Do you find the behavior of the first repro above intuitive for a newcomer?

I honestly don't see how a newcomer will figure out how refs work without reading documentation about it because they cannot just come up with the idea of:

  1. Adding a ref="a" on the template
  2. Returning a key with the same name on setup

So using ref or not here is more of a technical detail than being intuitive

It's also inconsistent with other stuff that updates the scope, like v-model. Why would v-model write to any property; but ref doesn't?

I'm sorry, I don't understand what you are exactly referring to

jods4 commented 4 years ago

I honestly don't see how a newcomer will figure out how refs work without reading documentation about it because they cannot just come up with the idea of:

Well, I did? (obviously I read some docs) This is feedback from a new Vue user: I tried to grab an element with ref and I had a bad time getting it to work. To make things worse, it's hard to debug why this doesn't work.

Disclosure: I'm trying out Vue but I have years of experience with several other frameworks and UI development in general. I may not be representative of a beginner. In Aurelia for example you can bind a property in your viewmodel to an element in DOM using the same syntax ref="vmProperty".

I'm sorry, I don't understand what you are exactly referring to

Here's a piece of code:

<input v-model="name" ref="box" />
setup() {
  return reactive({ name: 'John', box: null })
}

To me it's surprising (in a bad way) that name will be updated by the view; but box won't.

posva commented 4 years ago

Well, I did? (obviously I read some docs)

That's a bit contradictory: you didn't figure it out if you read the docs 😄 . Did you read this https://vue-composition-api-rfc.netlify.com/api.html#template-refs or something else? I don't think we have proper docs on this yet, so it's something we can gather feedback from

Thanks for the example, now I see how v-model and ref may look like they would behave the same way while they don't, and I think this is where the confusion comes from and it makes sense if there is no experience with Vue 2. Here is some context:

Regarding the technical difference, when using the reference to the element/component, it is reactive, meaning that we need to use a reactive variable to retrieve that ref in the js:

setup() {
  const el = ref(null)

  // notice the `value` like other reactive properties
  const textFromEl = computed(() => el.value.textContent)

  return { el }
}

This wouldn't be possible if we had el = null at the beginning since there is no way to modify the original variable.

Where in the code were you using the box/b2 reference? Do have a more concrete sample of what you were doing with that HTML element?

jods4 commented 4 years ago

Did you read this

I did but unfortunately I found it only after figuring out what was the problem on my own. That documentation is good but the ref caveat is really only mentionned once in passing:

[...], if a VNode's ref key corresponds to a ref on the render context, [...]

So it's there but it's easy to overlook.

I've read the source code and understand that the two directives work very differently under the hood. Forgive me if I'm a total noob but I have the impression it is so because of Vue 2 and how the API was before. When you say: "this.$refs.box with no property box declared anywhere" that applies to Vue 2, right? Because in Vue 3 there is no $refs anymore? So when you use the new Composition API, you have to provide a property named properly in the scope?

The current behavior makes a lot of sense in the old context of $refs but I think it's not most intuitive with the new API.

when using the reference to the element/component, it is reactive

I am fully aware of that. As I said in the original issue, many times an element won't change over the lifetime of your component, so that's fine. watch runs after the component is mounted, which is very convenient in this regard. (see example below)

More annoyingly, I might use reactive({ button: null }) to get a reactive element and this won't work either. Some of the available documentation currently promotes reactive({ x, y }) as an alternative to x = ref(), y = ref() but in this case it breaks.

Where in the code were you using the box/b2 reference? Do have a more concrete sample of what you were doing with that HTML element?

Sure, this is what I was experimenting with: https://codesandbox.io/s/little-microservice-lffu5

(The code editor doesn't load for me in FF, but it does in Chrome) You can collapse draw which is just painting logic and look at the remaining Vue glue.

You'll notice this code does a toRefs before returning the scope. It does so only to make the canvas work.

posva commented 4 years ago

the ref caveat is really only mentionned once in passing:

That's not true, it appears in all three code samples as well. The paragraph that mentions it is also the only explanation. That's rather hard to overlook.

When you say: "this.$refs.box with no property box declared anywhere" that applies to Vue 2, right? Because in Vue 3 there is no $refs anymore? So when you use the new Composition API, you have to provide a property named properly in the scope?

Yes, I'm referring to the difference from Vue 2 and Vue 3 trying to give some background to explain why this could make sense to us and make it clearer why it doesn't necessarily makes sense in its current state. No, this.$refs do not exist anymore.

I am fully aware of that. As I said in the original issue, many times an element won't change over the lifetime of your component

Sure, but we cannot be aware of that condition, so it has to be treated as the larger case: it could change -> it should be a reactive variable.

Thanks for the example, it gives more context! Instead of using toRefs, using ref(null) instead of null seems easier imo.

I agree that a warning makes total sense here. I still feel it's only a patch because of the comparison with v-model: the difference between what value is passed to ref is not as clear as it was in Vue2 even though one is a directive and the other is an attribute

jods4 commented 4 years ago

Thanks for the example, it gives more context! Instead of using toRefs, using ref(null) instead of null seems easier imo.

Yeah but I'm using reactive to avoid declaring several ref and having to use value. It's definitely weird to have to include one ref in the lot because of how it's used in the template.

posva commented 4 years ago

Yeah but I'm using reactive to avoid declaring several ref and having to use value. It's definitely weird to have to include one ref in the lot because of how it's used in the template.

Personally I didn't expect someone to return the object returned by reactive, in which scenario having aRef: null makes sense

jods4 commented 4 years ago

I wish this behavior would be changed, esp. since alpha 8.

In Vue 3 you don't think in refs names anymore. With the API you think in an object that represent a reactive state. So it's natural to think that ref='abc' in a template would set the abc property of your state to that element.

Forcing a ref here is not cool, e.g. you can also have reactive state by returning a reactive object:

setup() {
   return reactive({ element: null })
}

This code really doesn't make much difference compared to: return { element: ref(null) }.

The fix doesn't look complicted:

That's about it?

sionzee commented 4 years ago

The docs are really outdated or I have found a bug.

At Usage inside v-for they are showing how to put the elements to the array. But inside the ref function, you can't access the state.

:ref="el => { divs[i] = el }">

so here the divs variable is not defined.

Quick example:

<template>
  <p :ref="el => test.push(el)"></p>
</template>
...
setup() {
  return {test: ref([])};
}

Results in runtime-core.esm-bundler.js?5c40:223 ReferenceError: test is not defined.

Using "vue": "^3.0.0-alpha.8", "@vue/compiler-sfc": "^3.0.0-alpha.8".

Do you plan to put back something like $children? In my case I have manually defined 100 icons and need to foreach them all to run a function inside each specific component. Appending to 100 lines something like :ref="r => array.push(r)" looks quite weird to me.

// Edit, I just realized maybe the v-for is setting "this" context for template variables? If it does then it means I can't define multiple components to the same array. Hack will be to define foreach loop with 1 dummy iteration. (just checked it and it is not this case)

yyx990803 commented 4 years ago

@sionzeecz your case is working as intended. If you want to report a bug, open a new issue with actual reproduction instead of commenting on other issues.

sionzee commented 4 years ago

@yyx990803 I'm sorry for commenting under this issue. Anyway when you gave me already a response. If this works as is intended then the documentation is written wrongly? I can't spot almost any difference between code above and documentation.

Just found I can access _ctx. (where is the real ref) So it should be:

<template>
  <p :ref="el => _ctx.test.push(el)"></p>
</template>
...
setup() {
  return {test: ref([])};
}

At least this works as workaround (I don't think this is intended way to do it)

yyx990803 commented 4 years ago

By "working as intended" I mean the code you showed is working. There is no bug. If there is one, you should provide a reproduction (e.g. a codepen, and in a separate issue).

yyx990803 commented 4 years ago

Closing the original issue since a ref will always be required to use refs.

jods4 commented 4 years ago

@yyx990803 Can I ask what's the motivation? The changes I proposed seemed simple enough... why can't I put a DOM element reference into a reactive object?

yyx990803 commented 4 years ago

Because the user may do this:

const state = {
  foo: null
}

onMounted(() => {
  state.foo // expect to get a ref but won't, because local `state` is not reactive
})

return state

A ref is more consistent and more explicit.

jods4 commented 4 years ago

With the change I proposed it would totally work, wouldn't it? And that's kind of the point. I don't see how ref is more consistent or explicit. It may be for users coming from a Vue 2 background, I understand that and it should definitely work for them as they expect.

At the same time, Vue 2 is the past, Vue 3 is gonna have new users that never had exposure to Vue 2; or that might have experience with other frameworks. They might be surprised that your example (with or without reactive) doesn't work. Others framework have a ref keyword to grab a DOM element (e.g. Aurelia) and it just works on the model. That might create false expectations and frustrations. If aligning is possible, I think you should.

As a new user I genuinely don't understand the logic here and I feel this is a limitation more than an enhancement. After looking at the code, it seems rather trivial to change.