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

Ensure hydration scripts inside of slots render ASAP #4288

Merged
merged 4 commits into from
Aug 12, 2022

Conversation

matthewp
Copy link
Contributor

Changes

  • Fixes another race condition in rendering hydration script that's caused when both a slot and a child component contain hydrated scripts.
  • Since slots render to strings we need not render those scripts inside the slot, instead we inform renderPage() that the scripts are needed so that it can ensure they are added before any islands.

Testing

  • Test added for this race condition.

Docs

N/A, bug fix.

@changeset-bot
Copy link

changeset-bot bot commented Aug 12, 2022

🦋 Changeset detected

Latest commit: 733ed00

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

This PR includes changesets to release 13 packages
Name Type
astro Patch
@e2e/astro-component Patch
@e2e/error-react-spectrum Patch
@e2e/error-sass Patch
@e2e/errors Patch
@e2e/lit-component Patch
@e2e/preact-component Patch
@e2e/react-component Patch
@e2e/solid-component Patch
@e2e/solid-recurse Patch
@e2e/svelte-component Patch
@e2e/e2e-tailwindcss Patch
@e2e/ts-resolution Patch

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 Aug 12, 2022
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.

Great fix, thanks!

packages/astro/src/runtime/server/render/page.ts Outdated Show resolved Hide resolved
Co-authored-by: Nate Moore <natemoo-re@users.noreply.github.com>
@matthewp matthewp merged commit c218100 into main Aug 12, 2022
@matthewp matthewp deleted the hydration-race-slot branch August 12, 2022 16:18
@astrobot-houston astrobot-houston mentioned this pull request Aug 12, 2022
FredKSchott added a commit that referenced this pull request Aug 13, 2022
FredKSchott added a commit that referenced this pull request Aug 13, 2022
…#4302)

* Revert "Ensure hydration scripts inside of slots render ASAP (#4288)"

This reverts commit c218100.

* Create khaki-dots-press.md
@astrobot-houston astrobot-houston mentioned this pull request Aug 13, 2022
matthewp added a commit that referenced this pull request Aug 15, 2022
@matthewp
Copy link
Contributor Author

This PR had to be reverted to causing a regression. I have another, much safer, idea of how to fix that. Branch started here: https://github.com/withastro/astro/tree/slot-bug-1

@elevatebart
Copy link
Contributor

elevatebart commented Aug 16, 2022

Hello @matthewp,

Since this fix had to be reverted, could we re-open the other ticket until it is fixed again.
I only want this so that I can know when I can remove the hack I added (<Empty client:idle/>)

Thank you in advance,
Bart

@matthewp
Copy link
Contributor Author

Yes, thanks for the reminder.

@DevHusariaSolutions
Copy link

DevHusariaSolutions commented Sep 27, 2022

Hello guys, how's going? I've just ported app to vite-plugin-ssr for now, but I still hope You gonna make it work to let my SolidJS app works as You describe in docs :)
I've seen You merged new fix day before I published #4343 . Since preact is quite simple framework - I'm not so confident about SolidJS usage before tests for such frameworks. I know You have a lot of work, but I think that tests for framework listed in PRs related to some bug should be handled.
For now testing newer version of Astro bumped me in another issue... (some library instead getting ref of element, it gets:
{ t: "<div>...</div>" }
so probably hydration messes around here :/

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.

4 participants