Skip to content

fix(runtime-dom): fix getContainerType #12148

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

Closed
wants to merge 1 commit into from

Conversation

wucdbm
Copy link

@wucdbm wucdbm commented Oct 10, 2024

TL;DR async components with lazy hydration break when interacted with very quickly after hydration

The getContainerType function does not accept null, however in packages/runtime-core/src/hydration.ts:281 the following code const container = parentNode(node)! assumes container will not be null, but in fact, it is, and reading a field from it in getContainerType fails. I do not understand the need to use the non-null assertion operator throughout the code base of Vue, but I guess sometimes it can be OK for performance reasons.

Something else I do not understand is, after fixing getContainerType locally, we did not experience any further crashes (for now), and looking at the code, mountComponent expects to always get a container, but whenever that happens (container = null), and it does happen now and then since getContainerType used to crash in 1 of 5 test runs, no observable malfunctions happen. How can mountComponent still function properly when it requires a container, but gets passed a null?!

TypeError: Cannot read properties of null (reading 'nodeType')
    at getContainerType (https://acme.local:3000/node_modules/.vite/deps/chunk-TBLKTZOQ.js?v=11ed4a1a:3781:17)
    at hydrateNode (https://acme.local:3000/node_modules/.vite/deps/chunk-TBLKTZOQ.js?v=11ed4a1a:3943:13)
    at hydrateSubTree (https://acme.local:3000/node_modules/.vite/deps/chunk-TBLKTZOQ.js?v=11ed4a1a:7440:13) unknown {"name":"TypeError","message":"Cannot read properties of null (reading 'nodeType')","trace":"TypeError: Cannot read properties of null (reading 'nodeType')\n    at getContainerType (https://acme.local:3000/node_modules/.vite/deps/chunk-TBLKTZOQ.js?v=11ed4a1a:3781:17)\n    at hydrateNode (https://acme.local:3000/node_modules/.vite/deps/chunk-TBLKTZOQ.js?v=11ed4a1a:3943:13)\n    at hydrateSubTree (https://acme.local:3000/node_modules/.vite/deps/chunk-TBLKTZOQ.js?v=11ed4a1a:7440:13)"} Error: Uncaught TypeError: Cannot read properties of null (reading 'nodeType') @ File https://acme.local:3000/node_modules/.vite/deps/chunk-TBLKTZOQ.js?v=11ed4a1a:3781:17 https://acme.local:3000/wedding-rings/mens/platinum
Cannot read properties of null (reading 'nodeType')
TypeError: Cannot read properties of null (reading 'nodeType')
    at getContainerType (https://acme.local:3000/node_modules/.vite/deps/chunk-TBLKTZOQ.js?v=11ed4a1a:3781:17)
    at hydrateNode (https://acme.local:3000/node_modules/.vite/deps/chunk-TBLKTZOQ.js?v=11ed4a1a:3943:13)
    at hydrateSubTree (https://acme.local:3000/node_modules/.vite/deps/chunk-TBLKTZOQ.js?v=11ed4a1a:7440:13)
# 3780 through 3785
var getContainerType = (container) => {
  if (container.nodeType !== 1) return void 0;
  if (isSVGContainer(container)) return "svg";
  if (isMathMLContainer(container)) return "mathml";
  return void 0;
};
# 3927 through 3956
        } else if (shapeFlag & 6) {
          vnode.slotScopeIds = slotScopeIds;
          const container = parentNode(node);
          if (isFragmentStart) {
            nextNode = locateClosingAnchor(node);
          } else if (isComment(node) && node.data === "teleport start") {
            nextNode = locateClosingAnchor(node, node.data, "teleport end");
          } else {
            nextNode = nextSibling(node);
          }
          mountComponent(
            vnode,
            container,
            null,
            parentComponent,
            parentSuspense,
            getContainerType(container),
            optimized
          );
          if (isAsyncWrapper(vnode)) {
            let subTree;
            if (isFragmentStart) {
              subTree = createVNode(Fragment);
              subTree.anchor = nextNode ? nextNode.previousSibling : container.lastChild;
            } else {
              subTree = node.nodeType === 3 ? createTextVNode("") : createVNode("div");
            }
            subTree.el = node;
            vnode.component.subTree = subTree;
          }
# 7413 through 7450
  const setupRenderEffect = (instance, initialVNode, container, anchor, parentSuspense, namespace, optimized) => {
    const componentUpdateFn = () => {
      if (!instance.isMounted) {
        let vnodeHook;
        const { el, props } = initialVNode;
        const { bm, m, parent, root, type } = instance;
        const isAsyncWrapperVNode = isAsyncWrapper(initialVNode);
        toggleRecurse(instance, false);
        if (bm) {
          invokeArrayFns(bm);
        }
        if (!isAsyncWrapperVNode && (vnodeHook = props && props.onVnodeBeforeMount)) {
          invokeVNodeHook(vnodeHook, parent, initialVNode);
        }
        toggleRecurse(instance, true);
        if (el && hydrateNode) {
          const hydrateSubTree = () => {
            if (true) {
              startMeasure(instance, `render`);
            }
            instance.subTree = renderComponentRoot(instance);
            if (true) {
              endMeasure(instance, `render`);
            }
            if (true) {
              startMeasure(instance, `hydrate`);
            }
            hydrateNode(
              el,
              instance.subTree,
              instance,
              parentSuspense,
              null
            );
            if (true) {
              endMeasure(instance, `hydrate`);
            }
          };

@edison1105
Copy link
Member

Please provide a minimal reproducible demo. This will help us troubleshoot and identify the root cause of the issue. Error logs alone make it difficult for us to pinpoint the exact problem.

@edison1105 edison1105 added the need more info Further information is requested label Oct 11, 2024
@wucdbm
Copy link
Author

wucdbm commented Oct 13, 2024

Generally speaking, the exclamation marks are the main problem.

I will try to do so as soon as possible, but the minimal reproduction of this one is tough because of app routing and a myriad of things that need to be ported in it, that could be in a way or another leading to this.

And, by the way, #7086 this is still broken with latest Vue. The problem is exactly the same, just elsewhere. There's a reproduction repo for 2 years posted now, and noone seems to care enough. I need to know whether the half-working-day of porting our app's internal structure will go to waste or not.

@edison1105
Copy link
Member

@wucdbm

I think #7086 hasn't been resolved in two years likely because the reproduction demo is too complex. it contains many third-party libraries. This wastes a lot of time for contributors trying to help you find the bug. This is unreasonable because you are most familiar with your codebase, and if you can't extract a minimal reproduction demo, it's even more impossible for others to do so.

please see https://antfu.me/posts/why-reproductions-are-required

@wucdbm
Copy link
Author

wucdbm commented Oct 14, 2024

I can understand that reproductions are required, I am working in the same field in the end and face the same issues.

For #7086 the reproduction demo there is quite simple. Third party libraries are qutie irrelevant. I will then get rid of GraphQL and make a simple setTimeout() data fetcher. The thing of substance there is that the server does NOT see the hash parameter passed and consequently the server renders one thing, while the client expects to render something else due to the fact that it does see the query param and the UI must be different.

For this one, it'll take me a considerable amount of time to copy/pasta the project structure in terms nested transition/suspense/router-view/transition/suspense/keep-alive/suspense etc, to make an accurate representation of the conditions where it does break. And then, it breaks every 5th run of a test case. The test hovers over an component that is async with lazy hydration very quickly. Every now and then, the above happens.

@niksy
Copy link

niksy commented May 13, 2025

Getting same error but only in Safari. It’s really hard to reproduce it :/

@wucdbm did you managed to fix this locally?

@wucdbm
Copy link
Author

wucdbm commented May 14, 2025

@niksy Yes and No. More like no.

        plugins: [
            replace({
                'const getContainerType = (': `
                    const getContainerType = (e) => {
                        if (null === e) {return undefined}
                        if (e.nodeType !== 1) {return undefined}
                        if (isSVGContainer(e)) return 'svg'
                        if (isMathMLContainer(e)) return 'mathml'
                        return undefined
                    }\n
                    \n
                    const getContainerType123 = (`,
            }),

This one fixes the error in that it does not happen. But over my debugging sessions I managed to understand that the reason this happens is the first place is an error elsewhere, which somehow gets masked and this one gets to surface. Once you patch this with the above code, that underlying error will still happen, and will also render your app unusable, not responding to anything anyhow, just like if you didn't patch it.

We managed to figure out that the issue is with KeepAlive, which we have since ditched. It happens due to a weird combination of nested routes, transitions, keep alive, and two layers of suspense. To be fair, I'd ditch suspense as well as other things and do good old v-if loaders and simulate the suspense on the initial load via a plugin in the app that updates a ref.

The biggest problem here is that the vue codebase is littered with bullshit code with non-null assertions or whatever the F that evil thing with the exclamation mark is called. So now due to the lack of proper errors in place where this is "not supposed to happen, so we put an Fing ! in place", good luck have fun catching the root cause of this.

And it seems like the whole JS/TS community loves writing teenage code like that that smells from the get-go, so hmpf. What do I know 🤷 .

In light of the above (and also, thank you for reminding me about this one), I am closing this as it no longer makes sense.

@wucdbm wucdbm closed this May 14, 2025
@niksy
Copy link

niksy commented May 15, 2025

I still don’t know how to consistently reproduce the issue in Safari, currently I’ve managed to work around it and hope to return to it in the future or wait for some totally unrelated change in the codebase (or Safari update) fixes the issue 🤷‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need more info Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants