Skip to content
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(runtime-dom): fix getContainerType #12148

Open
wants to merge 1 commit into
base: main
Choose a base branch
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.

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.

2 participants