Partials rendering regression in 4.8.5

[Igloczek]

Astro Info

Astro                    v4.8.6
Node                     v20.12.2
System                   macOS (arm64)
Package Manager          pnpm
Output                   static (but it's broken with server + node.js adapter too)
Adapter                  none
Integrations             none

If this issue only occurs in one browser, which browser is a problem?

No response

Describe the Bug

Partials containing nested conditionally rendered elements are not properly rendering since 4.8.5

This is the minimal reproducible example I came with

---
export const partial = true
---

{
  true && (
    <>
      {true && <div>test</div>}
      {false && <div>test</div>}
      {true && <div>test</div>}
    </>
  )
}

Replacing those true and false with some real variables and functions leads to broken output too.

Astro >=4.8.5 returns 200 OK, with empty body, no errors in the console.

The same code, but not as partial works fine. The same code, but on 4.7.1 and 4.8.4 works fine.

What's the expected result?

I want to get either 200 with a proper HTML in the body, or 500 with error.

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-xu4kcf?file=src%2Fpages%2Fpartial.astro

Participation

• [ ] I am willing to submit a pull request for this issue.

[alex-sherwin]

Seeing same, as of 4.8.5 partial rendering doesn't work.

A response is generated with any headers etc, but body is never sent over the wire in the response.

But the render code for the body runs, it's just not used in the response seemingly.

[alex-sherwin]

Just looking at the diff for 4.8.4 to 4.8.5 https://github.com/withastro/astro/compare/astro%404.8.4...astro%404.8.5

The most likely change, I would think, is in the "Fix streaming in Node.js fast path" change here https://github.com/withastro/astro/commit/749a7ac967146952450a4173dcb6a5494755460c

In terms of actual, concrete behavioral differences made, this line stands out to me: https://github.com/withastro/astro/commit/749a7ac967146952450a4173dcb6a5494755460c#diff-d312d8fce663b920b083d795af0cf0ca4f20374f3467998906ec00ef2b85f6eaL280

Specifically noting that

next = promiseWithResolvers<void>();

Is no longer run in the new code as of 4.8.5

I'm not really familiar with this area of code at all, but, purely speculating from the overall changes this feels like the most likely culprit

[TheOtterlord]

Yep! Replicated locally and that is the cause. It looks like this only happens when the conditional content is the first top-level content in a component. Adding a {' '} to the top of the HTML markup returns the correct response (with a leading space).

[alex-sherwin]

Yep! Replicated locally and that is the cause. It looks like this only happens when the conditional content is the first top-level content in a component. Adding a {' '} to the top of the HTML markup returns the correct response (with a leading space).

I was just experimenting locally and I was thinking along the lines of (based on the change I highlighted) that it breaks only when the rendering is not completed in the current synchronous tick

I modified the astro package partials.test.js test and associated fixture to use the same Delayed component that various other text fixtures use and this indeed breaks it as well since it forces rendering to be async in a later tick

[alex-sherwin]

Ok, you're right.. now I'm thoroughly confused,

Even after fixing the line of code I mentioned..

This works:

---
import Delayed from '../../components/Delayed.astro';

export const partial = true;
---
{' '}
<Delayed ms={10} />

But this doesn't work

---
import Delayed from '../../components/Delayed.astro';

export const partial = true;
---
<Delayed ms={10} />

So.. the bug may be elsewhere.. feels like maybe the rendering code only attempts to resolve async rendering if it receives data on the first synchronous rendering pass?

Whatever the problem ends up being, i think the partials test needs more elaborate examples of sync/async rendering in the test suite

[alex-sherwin]

@TheOtterlord

Ah, you may have found it already, but, I didn't notice in the diff initially since it's obfuscated a bit by the typing changes

In 4.8.4 and before this line:

let next = promiseWithResolvers<void>();

Is actually assigning next via an invocation to promiseWithResolvers, and in 4.8.5 it's been changed to have a more explicit type signature, but the invocation of promiseWithResolvers was removed

So the initial invocation of promiseWithResolvers now only occurs once the AsyncIterator's next() function is first invoked, and this is clearly a fundamental difference from 4.8.4 and before

So, essentially, 4.8.4 and before call promiseWithResolvers on the initial process tick, and 4.8.5 and later first call promiseWithResolvers on a later process tick, and this seems to be the crux of the problem

[alex-sherwin]

So, now with a better understanding of the sequence of events...

I think in 4.8.5+ the problem is that next is never assigned until

if (!renderingComplete) {
    next = promiseWithResolvers();
}

But nothing ever await's that initial lazily assigned promise, changing to

if (!renderingComplete) {
    next = promiseWithResolvers();
    await next.promise;
}

Seems to fix the problem (however I'm not sure if Astro intends to await it here? but it probably should, i think...)

The old code assigned next outside the iterator, and the initial invocation of the AsyncIterator would first run await next

So something needs to await that initially assigned next.promise, either like my example above or rearrange the code so that the 4.8.5+ block

if (!renderingComplete) {
    next = promiseWithResolvers();
}

Comes before the

if(next !== null) {
    await next.promise;
}