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

Support streaming inside of slots #6775

Merged
merged 7 commits into from Apr 7, 2023
Merged

Support streaming inside of slots #6775

merged 7 commits into from Apr 7, 2023

Conversation

matthewp
Copy link
Contributor

@matthewp matthewp commented Apr 6, 2023

Changes

  • In order to support the Astro.slots.render('name') API previously renderSlot returned a string. This meant no streaming of slots.
  • Refactored that function to renderSlotToString. It's still used a bit internally.
  • Changed renderSlot, which is now just used inside the template (for <slot></slot>) to stream.
  • Depends on the compiler fix to preserve whitespace and newlines inside slots: Preserve whitespace in slots compiler#770
  • Closes Layout convention prevents HTML streaming #6561

Testing

  • Added test case to our existing streaming tests

Docs

N/A, bug fix

@changeset-bot
Copy link

changeset-bot bot commented Apr 6, 2023

🦋 Changeset detected

Latest commit: 4481566

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Apr 6, 2023
@matthewp matthewp marked this pull request as ready for review April 7, 2023 12:37
Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me, nice fix!

let chunk = decoder.decode(bytes);
chunks.push(chunk);
}
expect(chunks.length).to.be.greaterThan(2);
Copy link
Member

@ematipico ematipico Apr 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is 2 the expected the number of slots in the test fixture? Is it because we render two Wait components?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 is the expected. You get the first initial chunk. Then the chunk after the first wait. Then the chunk after the second wait. I guess I could change this to equal because it should always be 3.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might help. Having a strict equal can prevent possible regressions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for avoiding it is that Node buffers writes. If you send 2 chunks with no wait it will combine them into a single write on the network. So the chunks can be less than the number you expect, but never more. In this case, the wait is sufficiently long enough that it should always split into 3 chunks. SO I've updated the test to reflect that.

@matthewp matthewp merged commit fa84f1a into main Apr 7, 2023
14 checks passed
@matthewp matthewp deleted the slot-streaming branch April 7, 2023 15:04
@astrobot-houston astrobot-houston mentioned this pull request Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Layout convention prevents HTML streaming
3 participants