fix(ref): fix ref prop normalization

[Vanilagy]

Close #12029

Hey everyone!

I ran into inconsistencies with how template refs are treated between dev and prod here: https://github.com/vuejs/core/issues/12029

I have an element whose ref prop points to a non-ref object. Like this:

<script setup>
class A {}
const p = new A()
</script>

<template>
  <p ref="p">Hello</p>
</template>

While this is obviously useless code, it has happened to me and several of our colleagues during development, by accident. In dev, this fails silently and everything keeps working, making it hard to spot that you did something wrong. During production, the code fails and often nukes the entire component or page. This inconsistency is quite dangerous as it causes unintended production bugs on code that worked in development.

This is NOT the same issue as in https://github.com/vuejs/core/issues/11373 and https://github.com/vuejs/core/issues/4866 - these issues are concerned about dev/prod differences when it comes to dynamically-bound refs (:ref="x").

The issue starts at the fact that prod compiles the templates differently than dev. The props object passed to the p element in the dev version is { ref: "p" }. However, in the prod version it becomes { ref_key: "p", ref: p }. This is where the problem gets injected into the framework since we're passing p as the ref. This works just fine if p is actually a Ref, but it breaks down when it isn't. Since the compiler can't differenciate these cases statically, the fix must lie in the runtime logic.

The earliest spot I was able to catch this issue in was in normalizeRef. The ref that is passed in is typed as:

type _ =
  | null
  | string
  | Ref
  | ((
      ref: Element | ComponentPublicInstance | null,
      refs: Record<string, any>,
    ) => void)

Which led me to conclude that the current code was erroneous; if ref is non-null, but is also not a string, Ref, or function, then it is simply an invalid ref and should be changed to null. Correct me if this fix is incorrect; i.e. there is some configuration in which the "fallthrough case" (where ref is used as-is, despite being no string, Ref, or function) is intended.

[github-actions[bot]]

Size Report

Bundles

File	Size	Gzip	Brotli
runtime-dom.global.prod.js	101 kB (-10 B)	38 kB (-3 B)	34.2 kB (+12 B)
vue.global.prod.js	160 kB (-10 B)	58 kB (-2 B)	51.4 kB (-143 B)

Usages

Name	Size	Gzip	Brotli
createApp (CAPI only)	49 kB (-10 B)	18.9 kB (-5 B)	17.2 kB (-39 B)
createApp	55.6 kB (-10 B)	21.4 kB (-4 B)	19.5 kB (-13 B)
createSSRApp	59.6 kB (-10 B)	23.1 kB (-3 B)	21 kB (+12 B)
defineCustomElement	60.3 kB (-10 B)	22.9 kB (-3 B)	20.9 kB (+40 B)
overall	69.3 kB (-10 B)	26.5 kB (-2 B)	24.1 kB (+5 B)

[pkg-pr-new[bot]]

Open in Stackblitz

@vue/compiler-core

``` pnpm add https://pkg.pr.new/@vue/compiler-core@12031 ```

@vue/compiler-sfc

``` pnpm add https://pkg.pr.new/@vue/compiler-sfc@12031 ```

@vue/compiler-dom

``` pnpm add https://pkg.pr.new/@vue/compiler-dom@12031 ```

@vue/compiler-ssr

``` pnpm add https://pkg.pr.new/@vue/compiler-ssr@12031 ```

@vue/reactivity

``` pnpm add https://pkg.pr.new/@vue/reactivity@12031 ```

@vue/runtime-dom

``` pnpm add https://pkg.pr.new/@vue/runtime-dom@12031 ```

@vue/runtime-core

``` pnpm add https://pkg.pr.new/@vue/runtime-core@12031 ```

@vue/server-renderer

``` pnpm add https://pkg.pr.new/@vue/server-renderer@12031 ```

@vue/shared

``` pnpm add https://pkg.pr.new/@vue/shared@12031 ```

vue

``` pnpm add https://pkg.pr.new/vue@12031 ```

@vue/compat

``` pnpm add https://pkg.pr.new/@vue/compat@12031 ```

commit: 067a28f

[edison1105]

I don’t think this change is worthwhile.

First, this is incorrect usage. As mentioned in the documentation, developers should declare a ref to hold the element reference. This kind of incorrect usage can be caught during development because it is not an Element or a component instance when accessing p. see Playground

Secondly, this change only fixes the error in the production, but it does not make it work as expected. It also makes it harder for users to spot the issue since the error is no longer reported.

[Vanilagy]

I see where you're coming from; it can be argued that since I'm using Vue in an undefined way, Vue does not owe me any guarantees and can also then behave in undefined ways.

We have run into this issue several times at work, and perhaps it may help if I show the last instance of this error I encountered to clear up the nature of the issue:

---

It started like this. I was rendering raw HTML in a paragraph element, but then wanted access to the raw text representation of it (so, textContent). So I created a ref to the paragraph element:

<script setup lang="ts">
import { ref } from 'vue';
const html = ref('<b>Test</b>')
const p = ref<HTMLParagraphElement>()
// ...
</script>

<template>
  <p ref="p" v-html="html" />
</template>

However, then I realized I didn't need to rely on Vue to do this, and simply created an off-screen paragraph element instead, which I then work with in a watchEffect:

<script setup lang="ts">
import { ref, watchEffect } from 'vue';
const html = ref('<b>Test</b>')
const p = document.createElement('p')
watchEffect(() => { /* ... */ })
// ...
</script>

<template>
  <p ref="p" v-html="html" />
</template>

This code now works as intended and is what we kept. My oversight here was not removing the ref="p" at the bottom (it is not needed anymore) - however, in dev, not only did my component work flawlessly, but there was also no warning in the console and no error in the typecheck or IDE.

Perhaps this clears up the usage for you: The code works as intended because p is used as it is defined in the script, and it not expected that p is overridden or set by the ref in the template below. It is simply that there is a faulty ref in the template that does nothing and is no longer needed.

The logic in the script is correct, and the ref in the template is wrong - not the other way around.

---

If the behavior is to be kept like this (where this code can nuke an entire page in prod), then I think the error should also be equally as severe in development where it can be caught early by the developer. But since dev's current behavior is to do nothing (which I also think is fine), I argue that prod's behavior should not be more severe, since prod is a more high-stake environment. So that means prod should also do nothing, which is what the change in this PR achieves.

[edison1105]

OK, I fully understand the situation you're facing. I think we should keep DEV and RPOD consistent. reopen https://github.com/vuejs/core/issues/12029

[yyx990803]

https://github.com/vuejs/core/issues/12029#issuecomment-2378234245

[Vanilagy]

I agree that adding a warning for this case is definitely good as it nudges the dev to correct their mistake.

However, regardless of the issue at hand, is there anything incorrect about the code change in this PR? More specifically, could you give an example of when this branch of the code is taken and makes sense:

(excuse the screenshot)

Because that branch being taken would imply that the incoming ref is already of the type VNodeNormalizedRefAtom (so an object with properties i, r, k and f). When is that the case?

[yyx990803]

This can happen when a user manually assigns the ref property of an existing, already normalized VNode into the props of another h or createVNode call. It's unlikely to happen in template-based usage but is possible in projects using manually written render functions.