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

feat: explain the limitations of serialization #10944

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

branberry
Copy link
Contributor

@branberry branberry commented Feb 12, 2025

Description (required)

Documents the limitation of props for framework components, and server island components.

Related issues & labels (optional)

Copy link

netlify bot commented Feb 12, 2025

Deploy Preview for astro-docs-2 ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit a309d6b
🔍 Latest deploy log https://app.netlify.com/sites/astro-docs-2/deploys/67bdcf9f197668000851300a
😎 Deploy Preview https://deploy-preview-10944--astro-docs-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@astrobot-houston
Copy link
Contributor

astrobot-houston commented Feb 12, 2025

Lunaria Status Overview

🌕 This pull request will trigger status changes.

Learn more

By default, every PR changing files present in the Lunaria configuration's files property will be considered and trigger status changes accordingly.

You can change this by adding one of the keywords present in the ignoreKeywords property in your Lunaria configuration file in the PR's title (ignoring all files) or by including a tracker directive in the merged commit's description.

Tracked Files

File Note
en/guides/framework-components.mdx Source changed, localizations will be marked as outdated.
en/guides/server-islands.mdx Source changed, localizations will be marked as outdated.
Warnings reference
Icon Description
🔄️ The source for this localization has been updated since the creation of this pull request, make sure all changes in the source have been applied.

@sarah11918
Copy link
Member

Thanks for jumping on this issue, Brandon! 🙌 It's looking pretty good, but I think this is now verging into "not a caution" territory, so I'm going to think about about making it just regular body text or at most a "note." Just waiting for some other feedback on this, but very happy to have this!!

@sarah11918 sarah11918 added the improve or update documentation Enhance / update existing documentation (e.g. add example, improve description, update for changes) label Feb 14, 2025
@branberry branberry requested a review from sarah11918 February 19, 2025 11:00
@branberry
Copy link
Contributor Author

Hey Sarah! I rewrote the middle sentence, and I moved it around a bit. Hopefully that seems to be a meaningful improvement!

Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Thank you for taking another pass at the older, existing content that I do think can be improved. It's very helpful!

So given what you proposed, I tried to dig even a little deeper. I do not know that what I wrote is correct, but I think it's the structure/level of detail I'd like to have here. So if you can help make the content actually correct, then I think we have a winner!

Copy link
Member

@Fryuni Fryuni left a comment

Choose a reason for hiding this comment

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

The new text for client islands looks great and is technically correct. Good job @branberry !!

I left some comments about the server islands page, mostly it needs the same improvements as the other page.

@@ -33,6 +33,15 @@ const avatarURL = await getUserAvatar(userSession);
---
<img alt="User avatar" src={avatarURL} />
```
:::caution[Server Island Prop Limitations]
You can pass a function as a prop to a server island component, but this only works during server rendering. If you try to use the function in a hydrated component (for example, as an event handler), an error will occur.
Copy link
Member

Choose a reason for hiding this comment

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

You can pass a function as a prop to a server island component, but this only works during server rendering.

Server islands are always server rendered. That is their purpose, make something server rendered even on a statically pre-rendered page. So this seems a bit unhelpful.

Functions can be passed as props while on the server, they can't be passed to the components marked with server:defer. Meaning you can pass functions around on a server island, but never to a server island.

@@ -33,6 +33,15 @@ const avatarURL = await getUserAvatar(userSession);
---
<img alt="User avatar" src={avatarURL} />
```
:::caution[Server Island Prop Limitations]
You can pass a function as a prop to a server island component, but this only works during server rendering. If you try to use the function in a hydrated component (for example, as an event handler), an error will occur.
Copy link
Member

Choose a reason for hiding this comment

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

If you try to use the function in a hydrated component (for example, as an event handler), an error will occur.

This example seems weird to me. Server islands are not hydrated (at least not in the sense we use anywhere else in the docs for that word, it doesn't add handlers and bindings for interactivity to existing DOM nodes). They get replaced with the content of a separate request when loaded.

The fallback/original content doesn't require any hydration and neither does the code after the swap. After the swap there isn't even any JS related to the server island in the document, contrary to hydration, which is also a win for server island, and might give people the idea that server islands are heavier on their site than it really is.

:::caution[Server Island Prop Limitations]
You can pass a function as a prop to a server island component, but this only works during server rendering. If you try to use the function in a hydrated component (for example, as an event handler), an error will occur.

This is because functions cannot be _serialized_ (transferred from the server to the client) by Astro. Objects with circular references are also not serializable.
Copy link
Member

Choose a reason for hiding this comment

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

I think this should have the same wording regarding serialization as the client islands. Even more so here to not say that the limitation is regarding sharing information with the client because that is an implementation detail. It could send the information between instances of the server (which might remove some other limitations we currently have) and it would have the same problem.

@sarah11918
Copy link
Member

Thanks for this additional helpful context, Fryuni!

(And nice job to get a "technically correct" out of Fryuni, @branberry ! 😄 Would you be willing to tackle the extra sections mentioned above?)

@branberry
Copy link
Contributor Author

Hey @sarah11918 and @Fryuni, thank you so much for the thorough code review, I really do appreciate it. It's been incredibly helpful, and I've learned a good deal. Working on those extra sections now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improve or update documentation Enhance / update existing documentation (e.g. add example, improve description, update for changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document limitations of island prop serialization
4 participants