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.

Sign up for GitHub

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

Jump to bottom

feat: Add support for server side Portals #167

Merged
merged 14 commits into from
Oct 6, 2024
Merged

feat: Add support for server side Portals #167

merged 14 commits into from
Oct 6, 2024

Conversation

Fryuni
Copy link
Owner

@Fryuni Fryuni commented Oct 1, 2024 •
edited
Loading

Working implementation of withastro/roadmap#1032 (comment)

Still missing the tests to be ready

  • <portal-in to="some name"> elements are voided and their children sent to a matching portal named "some name"
  • <portal-out name="some name"> are replaced flatly with the children sent from <portal to="some name"> (the same effect as .replaceWith())
  • Implicit portal at start and end of <head>, <body> and every element with a target

@Fryuni Fryuni self-assigned this Oct 1, 2024
@vercel Vercel
Copy link

vercel bot commented Oct 1, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
inox-tools ✅ Ready (Inspect) Visit Preview Oct 6, 2024 5:04pm

@Fryuni Fryuni changed the title feat: Add support for server side Portals across the document feat: Add support for server side Portals Oct 1, 2024
@Fryuni
Copy link
Owner Author

Fryuni commented Oct 1, 2024

Resulting HTML from changed demo can be seen here:
https://feat-portal-gun.inox-tools-ex-request-nanostores.pages.dev/

image

@florian-lefebvre
Copy link
Collaborator

That looks pretty cool! A few comments from poking at at the code:

  1. I'm not sure using a portal tag is a good idea, given that it has already usage in HTML. You could mabe provide some components that use data attrs under the hood or something?
  2. I think this disables streaming? If so, worth mentioning in the docs

@Fryuni
Copy link
Owner Author

Fryuni commented Oct 1, 2024 •
edited
Loading

That looks pretty cool!

Thank you, kind sir 😊
It was fun

  1. I'm not sure using a portal tag is a good idea, given that it has already usage in HTML.

Yes, I also found that out. Technically, any tag without a - is reserved by the HTML spec for future use. That is why custom elements for web components must have a -.

Astro already overrides the semantics of <slot>, <script> and <style>. I think doing the same for <portal> is fine as long as we include an opt-out in the form of is:inline like the others.

You could maybe provide some components that use data attrs under the hood or something?

The advantage of using a direct HTML tag for this is that it works even when the element was added by an UI framework component.

The disadvantage is that makes such UI framework components don't work correctly on the client side. But as an alternative, users can put the entire UI framework in the portal, which will move the entire Astro Island to the new place and be hydrated there.

  1. I think this disables streaming? If so, worth mentioning in the docs

Yes, I mentioned that in my comment on the discussion withastro/roadmap#1032 (comment)
By the concept of how the portal should work I don't think that can be avoided. It will be in the docs.

@Fryuni Fryuni marked this pull request as ready for review October 5, 2024 21:33
florian-lefebvre
florian-lefebvre reviewed Oct 6, 2024
View reviewed changes
packages/portal-gun/README.md Outdated Show resolved Hide resolved
packages/portal-gun/package.json Show resolved Hide resolved
packages/portal-gun/package.json Show resolved Hide resolved
packages/portal-gun/src/internal/debug.ts Show resolved Hide resolved
packages/portal-gun/virtual.d.ts Outdated Show resolved Hide resolved
@Fryuni Fryuni merged commit fccaa43 into main Oct 6, 2024
20 checks passed
@Fryuni Fryuni deleted the feat/portal-gun branch October 6, 2024 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@florian-lefebvre florian-lefebvre florian-lefebvre left review comments

@OliverSpeir OliverSpeir Awaiting requested review from OliverSpeir

Assignees

@Fryuni Fryuni

Labels
pkg/portal-gun
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Fryuni @florian-lefebvre