-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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(ref): fix ref prop normalization #12031
Conversation
…ev and prod build behavior
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-sfc
@vue/compiler-dom
@vue/compiler-ssr
@vue/reactivity
@vue/runtime-dom
@vue/runtime-core
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
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 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. |
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, <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 <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 Perhaps this clears up the usage for you: The code works as intended because 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. |
OK, I fully understand the situation you're facing. I think we should keep DEV and RPOD consistent. |
Co-authored-by: edison <daiwei521@126.com>
This can happen when a user manually assigns the |
Close #12029
Hey everyone!
I ran into inconsistencies with how template refs are treated between dev and prod here: #12029
I have an element whose
ref
prop points to a non-ref object. Like this: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 #11373 and #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 thep
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 passingp
as the ref. This works just fine ifp
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: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 tonull
. 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.