Appending to innerHTML in a directive appears to break rendering

Pezmc commented 1 year ago

Vue version

3.2.45

Link to minimal reproduction

https://github.com/Pezmc/vuejs-append-to-innerHTML-issue

Steps to reproduce

git clone https://github.com/Pezmc/vuejs-append-to-innerHTML-issue
npm install
npm run dev

Click the toggle button

What is expected?

Vue.js should continue to be able to run render updates even after content has been appended to the HTML Or throw a clearer error message warning not to modify innerHTML.

What is actually happening?

Vue.js appears to crash entirely throwing the below error. This breaks reactivity not only the current Component, but all components on the page.

Error: TypeError: Cannot read properties of null (reading 'insertBefore')

Section of Vue.js code that throws the error:

const nodeOps = {
    // Both parent and anchor are null
    insert: (child, parent, anchor) => {
        parent.insertBefore(child, anchor || null);
    },

System Info

System:
    OS: macOS 13.0
    CPU: (10) arm64 Apple M1 Max
    Memory: 3.92 GB / 64.00 GB
    Shell: 3.5.1 - /opt/homebrew/bin/fish
  Binaries:
    Node: 16.17.0 - ~/.volta/tools/image/node/16.17.0/bin/node
    Yarn: 1.22.19 - ~/.volta/tools/image/yarn/1.22.19/bin/yarn
    npm: 8.15.0 - ~/.volta/tools/image/node/16.17.0/bin/npm
  Browsers:
    Brave Browser: 107.1.45.131
    Chrome: 107.0.5304.110
    Firefox: 105.0.1
    Safari: 16.1
    Safari Technology Preview: 16.4
  npmPackages:
    vue: ^3.2.45 => 3.2.45

Any additional comments?

Perhaps there is a better best practice for adding data to the DOM in a directive.

I originally came across this as we're using a directive to append a tooltip that shows on hover to the DOM.

For what it's worth it seems that using append() rather than innerHTML += sidesteps the issue (see https://github.com/Pezmc/vuejs-append-to-innerHTML-issue/compare/vuejs-append-to-dom-sidestep?expand=1)

LinusBorg commented 1 year ago

Manually messing with the DOM that is controlled by Vue is practically never a good idea.

The only safe things you can do are, pretty much:

inserting stuff into an otherwise totally empty element
adding attributes to an element that are not present on the element in the template.

Anything else is practically guaranteed to break in one situation or another, because you are making Vue's virtualDOM become out of sync with the actual DOM.

Giving a better error message is also kinda hard as we have no way of tracing the problem back to your imperative DOM change in some directive.

Pezmc commented 1 year ago

Thanks for the reply @LinusBorg, you make very good points!

Some follow-up questions:

Is there a best practice for how to implement something like a tooltip?
Is there somewhere suitable to ocument in Vue.js docs that this should not be done
How does the v-html directive work if it's not manipulating the DOM?
Is there any way to append to the DOM & keep the virtual DOM in sync?

Re the error, is there any other circumstance where the parent node ends up null? Could parentNode == null throw a "cannot find the parent node for XYZ component or node, is the dom out of sync?" or similar error?

LinusBorg commented 1 year ago

Is there a best practice for how to implement something like a tooltip?

With a component, usually. Not a custom directive.

Is there somewhere suitable to document in Vue.js docs that this should not be done

Likely somewhere at the end of this introduction section, the info-box already goes a bit in the right direction. https://vuejs.org/guide/reusability/custom-directives.html#introduction

How does the v-html directive work if it's not manipulating the DOM?

It is manipulating the DOM. But you you usually use it kind of like this, inserting stuff into empty elements:

<article v-html="aBunchOfHTMLCompiledFromMarkdown"><!-- totally empty element --> </article>

Is there any way to append to the DOM & keep the virtual DOM in sync?

if the surrounding DOM is stable, it's likely safe. But i won't go as far and promise that.

Re the error, is there any other circumstance where the parent node ends up null? Could parentNode == null throw a "cannot find the parent node for XYZ component or node, is the dom out of sync?" or similar error?

Well, the immediate reason for this error (and others like it, from other DOM operations Vue does) is vdom being out of sync, usually. could be because of something the user does, like in your case, or because of a bug in Vue's renderer, or the transition component, etc.

We could think about providing a generic warning like the one your propose here alongside the real error, but that would require wrapping each DOM change operation in its own try/catch block, and I'm not sure if that's not having perf effects on its own, seeing how this is done thousands upon thousands of times on the critical render path. No idea.

vuejs / core