🏖 Portal - #3767
🏖 Portal#3767oliverlloyd wants to merge 5 commits into
Portal#3767Conversation
| interface Props { | ||
| componentName: string; | ||
| when?: 'immediate' | 'idle' | 'visible'; | ||
| props?: any; |
There was a problem hiding this comment.
Is there a way I can use componentName to get the expected type and use that here? This seems pretty wild but would really help improve the dev ex!
There was a problem hiding this comment.
There was a problem hiding this comment.
You mean read it from the existing files? Yes, but you’d have to have some sort of process that generates a declaration file from reading the filesystem, I think.
That’s what the interview tool does in build.ts
There was a problem hiding this comment.
Is it a good idea? It's hard for me to estimate the trade off between not having things typed - which adds friction when inserting a new Portal - and implementing a special solution to make typing work - which might be lots of work and potentially brittle / hard to maintain?
There was a problem hiding this comment.
I don’t think it’s a lot of work, and that file has another added benefits: It’s a live document containing all the current portals. I’m happy to set half-an-hour aside to have a crack at this.
The question that remains is how the generation steps gets called, but I’m sure there are precendents… make portals?
There was a problem hiding this comment.
🤔 I'm not 100% following your intention but would what you're thinking of solve the dev ex problem? That is, I want to insert a new Portal and type
<Portal componentName="MyThing" ...
And then when I go to enter the props they are already typed as needed?
|
Size Change: +32 B (0%) Total Size: 3.16 MB
ℹ️ View Unchanged
|
| [k: `${string}Variant`]: "variant"; | ||
| [k: `${string}Control`]: "control"; | ||
| [k: `${string}Variant`]: 'variant'; | ||
| [k: `${string}Control`]: 'control'; |
| name: string; | ||
| when?: 'immediate' | 'idle' | 'visible'; | ||
| props: any; | ||
| children: React.ReactNode; |
There was a problem hiding this comment.
children is for when we use a Placeholder
| * We expect the element to always be a `gu-*` custom element | ||
| * | ||
| * @param marker : The html element that we want to read the name attribute from; | ||
| * @param element : The html element that we want to read the name attribute from; |
There was a problem hiding this comment.
Respecting a comment made by @sndrs over in libs for consistency
| import { getName } from './getName'; | ||
| import { getProps } from './getProps'; | ||
| import { getName } from '../getName'; | ||
| import { getProps } from '../getProps'; |
There was a problem hiding this comment.
I moved these to the common root folder so they can be shared. We can consider a different folder structure, this just worked for now
| // that will not cause the child component being passed in to be mounted | ||
| // again. Instead it is simply rerendered | ||
| // https://github.com/preactjs/preact/blob/df748d106fb78fbd46d14563b4712f921ccf0300/compat/src/portals.js | ||
| export const LegacyPortal = ({ rootId, children }: Props) => { |
There was a problem hiding this comment.
This file was renamed without edits
| * @param placeholderHeight - Height in pixels. If provided, a Placeholder is server rendered | ||
| * | ||
| */ | ||
| export const Portal = ({ |
There was a problem hiding this comment.
The Naming Things is Hard question: is it confusing to repurpose the name Portal, which is a first-class concept in (P)react that does something similar-ish but not really?
Without fully understanding the purpose of this pattern, would a more descriptive name be something like RenderOnClient? As in:
<RenderOnClient
componentName="Onwards"
when="visible"
props={{
onwardsUrl: CAPI.onwardUrl,
format,
}}
placeholderHeight={400}
/>There was a problem hiding this comment.
I guess what this component is doing is marking a place in the server side rendered content and saying, put some html here on the client using these attributes.
I quite like RenderOnClient although it is perhaps a bit verbose? What about Render? That nicely aligns with the ReactDOM.hydrate vs ReactDOM.render.
There was a problem hiding this comment.
Cool, thanks @oliverlloyd, I found this helpful in clarifying the purpose of this component 🙂
You're right that Render does align nicely with that React method, assuming people realise that this component name is a reference to that. Personally, I see the word Render appearing more and more, and hence meaning less and less! The naming of React's render method hasn't scaled well, which is why we see more verbose names like renderToString and renderToStaticMarkup crop up later. This is why I prefer the more verbose name.
I'm not sure if that's a me thing, and I would be interested to hear other opinions.
There was a problem hiding this comment.
Yeah, totally. I'm like you, I have my preference but that's based on my own feelings. Render is aligned and concise but it could could also be confusing.
A better way to choose a name should be with wider input and a focus on what will make sense to new people coming to the code with less context.
Aside. It's perhaps interesting to think about how Astro solved this problem. They continued to use their client api and incorporated this scenario into that. That wouldn't work for as but maybe there are other examples for similar abstractions we can align with?
There was a problem hiding this comment.
Thinking this api through some more, I'm starting to think a better approach might be to fold this logic into Hydrate and not have Portal at all. We simply could ask Hydrate to not always server render it's children, which essentially has the same effect as what this PR is proposing.
The api for Hydrate might look something like
<Hydrate when="immediate" where="client">
<MyClientOnlyThing />
</Hydrate>or
<Hydrate when="immediate" ssr={false}>
<MyClientOnlyThing />
</Hydrate>|
I'm marking this PR as draft while we consider if we want to change the Hydrate api instead |
|
Closing in favour of #3784 |
What does this change?
This PR introduces a new way to add portals to a page. A 'Portal' is the name given to any content added to the page on the client. This is similar concept to an island, but with the difference that the content is not server side rendered.
Why?
The primary goal for this work is to improve the developer experience of teams working on DCR. Adding content client side previously required an understanding of the wider app architecture and edits to several files. This is now reduce to one api in one location.
In addition, we're making it easier to move away from the
App.tsxpattern as discussed here.Only send the data required
By serialising the props for each component alongside its declaration on the server and not in a global window object, we ensure we only send the data that is needed.
Lazy insertion
Using the same api that was introduced for lazy hydration, we can now download and render components when visible, deferring content below the fold until scrolled into view. We can also tell the browser to render less important content when it is idle, preventing the thread from being blocked.
LegacyPortalI wanted to use the
Portalnamespace and also didn't want two things with similar names confusing people so I've sort of deprecated the old component by calling itLegacyPortaland stolen it's nameUsage
This is the only code developers need to write to have their content inserted on the client*
*You also need to rename the component being used to include 'importable'. For example,
Onwards.importable.tsx.