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

[bug]: empty result of lazy component doubles next node #4075

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aleksandrjet
Copy link

I added test, which shows that empty result of Lazy component doubles next node while hydration.

<main>
  <Suspense fallback={<p>will be never showed while hydration</p>}>
    <Lazy /> <!-- if it returns null, the next dom node will be doubled -->
  </Suspense>
  <p>will be rendered twice after hydration</p>
</main>

This seems, it happens because error, when rendering a lazy component:

// compat/src/suspense.js:261
if (!component) {
  throw prom;
}

Maybe we incorrect handle this error. I don't understand, why we need to remove last node from excessDomChildren and assing a value of oldDom to newVNode._dom

// src/diff/index.js:260
catch (e) {
  newVNode._original = null;
  if (isHydrating || excessDomChildren != null) {
    newVNode._hydrating = !!isHydrating;
    
    // We think, that oldDom will be replaced
    // a new node, which will be returned by Lazy. 
    // But this may be not so, because Lazy can return null
    newVNode._dom = oldDom;
    excessDomChildren[excessDomChildren.indexOf(oldDom)] = null;
  }
}

I tried to add additional handling for this error, but it breaks some of tests for suspense-hydration, despite the fact current test begun work

// src/diff/index.js:260
catch (e) {
  newVNode._original = null;
  if (isHydrating || excessDomChildren != null) {
    newVNode._hydrating = !!isHydrating;
    
    if (newVNode.type.displayName !== 'Lazy') {  // it breaks other tests
      newVNode._dom = oldDom;
      excessDomChildren[excessDomChildren.indexOf(oldDom)] = null;
    }
  }
}

I am looking for a solution and i need help of community. Now i think it can not fix by additional error handling, because at moment of processing node when hydration we don't know, what will return Lazy

P.S. I understand, that lazy component with empty result is not the best idea. But it give unpredictable result with doubles nodes. I think, it should working or throw error

@JoviDeCroock
Copy link
Member

JoviDeCroock commented Jul 10, 2024

Basically the reason for reserving the node during hydration is that we need a node to continue hydrating from when the Suspense boundary resolves, we can't predict that the result will be empty so this is kind of a necessity, we could maybe do with a heuristic where we add a _reserved property that can be consumed when we see more Virtual DOM Nodes then we see real DOM nodes. For instance this error would happen as well if your suspended component would return null as its render value.

I have been looking at the reverse case in #4438 where rather than 1 DOM-node being returned, a Fragment is returned with multiple DOM nodes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants