Support for `teleports` with Vue SSR

pdanpdan commented 1 month ago

Hello.

When using vike-vue the general structure of HTML is fixed in onRenderHml and in order to add support for teleport in SSR (https://vuejs.org/guide/scaling-up/ssr.html#teleports) one needs to:

pass a context (ctx) to render... functions
add the content of ctx.teleports in various places in HTML

Is there a way to do this while still using vike-vue or a full custom implementation is required?

brillout commented 1 month ago

@4350pChris WDYT?

pdanpdan commented 1 month ago

Looking at the code I think creating a ctx, paying it to render functions and allowing a custom templates string would be enough.

4350pChris commented 1 month ago

Working on it. See my PR #88

I'm guessing ids for the teleport roots should be enough. Let me know if this would work for you, then I'll release it ASAP.

pdanpdan commented 1 month ago

ctx.teleports is an object and the keys are the selectors used by each teleport. With the implementation in the PR they would all go at the end of BODY, but a real world example might have situations where you teleport a header at start of NODY and the modals at the end of BODY, or nested structures of teleport targets.

I think the user should be allowed to select where to place each teleported fragment, and that would be most easily done by allowing user to change the HTML template - it's hard to make a configuration that would cover all situations.

And maybe it would be better to pass the ssrContext also here headHtml = dangerouslySkipEscape(await renderToStringWithErrorHandling(app)) - not related to teleports but I just feel it would be more future proof.

4350pChris commented 1 month ago

You're right, I never even thought about having nested teleport targets. Also, we could support teleports to body without any user config.

However, wouldn't supporting a nested data structure be enough? I can't think of a use case where you'd have custom html within a teleport target. So maybe just having beforeAppTeleportIds and afterAppTeleportIds would be enough, given they support arbitrary nesting?

pdanpdan commented 1 month ago

By nested I was thinking more about something like this:

<body>
  ...
  <div class="teleports-container">
    <div id="teleport-modals"></div>
    <div class="teleport-notifications-container">
      <div id="teleport-notifications-top"></div>
      <div id="teleport-notifications-bottom-left"></div>
      ...
    </div>
  </div>
</body>

4350pChris commented 1 month ago

Yes, I was thinking of turning this into ['teleports-container', ['teleport-modals', 'teleports-notifications-container', ['teleport-notifications-top']]]

Looking at it now though, this might get a little confusing.

Edit - I just saw that one has a class so this doesn't work.

However, wouldn't it make more sense to have a plain vue notification component that has this structure and only teleporting that component? Afaik that would allow using the divs contained within that component as targets as well.

pdanpdan commented 1 month ago

It was just a quick example to show a possible situation where the teleport targets have a non-trivial structure.

brillout commented 1 month ago

by allowing user to change the HTML template

The issue with that is that it doesn't work well with Vike extensions, as it would lead to conflicts if two Vike extensions expect a different HTML template. The long-term goal is to have Vike extensions for the most widespread tools, for zero-config (but ejectable) integrations.

That said, an option would be to create new settings bodyHtmlStart and bodyHtmlEnd. These can be made cumulative, so that Vike extensions don't conflict with each other.

Speaking of cumulative, if we go for the teleportIds solution, I think we should make it cumulative. (@4350pChris FYI cumulative now also supports hooks, which I'll release as soon as I'm done with the head management redesign.)

What I like about teleportIds is that it's clear what it's about. On the other hand bodyHtml{Start,End} is more generic, potentially covering more use cases. Also, bodyHtml{Start,End} could maybe also be implemented by vike-{react,solid} :eyes:.

pdanpdan commented 1 month ago

bodyHtmlStart/End sounds good to me

brillout commented 1 month ago

Actually, there is another use case for bodyHtml{Start,End}:

I stuck my tracking scripts at the bottom of +Layout.tsx, but ideally there'd be a way to inject them outside of React.

(Quote coming from a feedback about vike-react.)

So maybe we should go for bodyHtml{Start,End}? @4350pChris WDYT?

pdanpdan commented 1 month ago

At least for teleports the body start and end strings should be functions with access to ssrContext.

brillout commented 1 month ago

I guess you mean pageContext. Curious: why do you want to access pageContext?

pdanpdan commented 1 month ago

I meant the ctx passed to renderTo... functions (in the PR it's called ssrContext)

brillout commented 1 month ago

That makes sense.

4350pChris commented 1 month ago

I'll try to work on it and get a PR ready by tomorrow.

brillout commented 1 month ago

Actually, I wonder whether vike-vue could inject ssrContext.teleports automatically? Vue provides the list of teleport IDs. No need for the user to set anything for simple use cases? For advanced use case, maybe an extra setting to tell vike-vue to not inejct certain teleport <div> wrappers.

4350pChris commented 1 month ago

That's basically what I tried doing first. However, this only includes teleports that are used during SSR. This leads to teleports not working after client side navigation if the container they're targeting is not present during SSR. So, while this would work in some cases I feel like this would also lead to unexpected errors and frustration as this wouldn't be obvious for users when they first encounter it.

Nuxt, for example, only supports teleports to body. However, the official Vue guide actually recommends against doing this. Therefore, I don't think that offering the same default as Nuxt is the way to go, even though its implementation would be very straightforward.

brillout commented 1 month ago

I see. Maybe manipulating the DOM in onRenderClient() before rendering the new page could be an option :thinking:

4350pChris commented 1 month ago

I don't see a way we could have information about used teleports at that point, since that comes from the SSR context.

brillout commented 1 month ago

I see.

I guess it‘s a Vue design flaw, that it doesn’t provide that information without SSR.

4350pChris commented 1 month ago

Indeed. The teleport API is technically not even stable yet, even though it's been in use for some while.

As for this PR - how about providing a default bodyHtmlEnd function that injects a single div, say <div id="teleport"></div>? For most use cases it's probably fine to inject into a single div at the end of the body. I feel this hinges on the documentation more than anything else.

brillout commented 1 month ago

I ain’t that familiar with Vue but sounds good to me. @pdanpdan WDYT?

pdanpdan commented 1 month ago

A sample tamplate would look like this when using vike without vike-vue:

const documentHtml = escapeInject`<!DOCTYPE html>
<html lang="${ lang }" dir="ltr">
  <head>
    <meta charset="UTF-8" />
    ${ faviconTag }
    ${ titleTag }
    ${ descriptionTag }
    ${ headHtml }
  </head>
  <body>
    <div id="app">${ pageView }</div>
    <div id="modals-holder">${ dangerouslySkipEscape(ctx.teleports?.[ '#modals-holder' ] || '') }</div>
  </body>
</html>`;

pdanpdan commented 1 month ago

I ain’t that familiar with Vue but sounds good to me. @pdanpdan WDYT?

From my point of view the most flexible (and simple to use) would be the method with bodyHtml{Start,End} as functions that receive the ctx (ssrContext) as parameter.

I see another solution using strings and template markers and doing the replacement in vike-vue, but it would look too much like magic because you must replace the markers that are present in ctx.teleports but also the markers that are not present should be replaced with ''.

brillout commented 4 weeks ago

@pdanpdan Merged and released in 0.7.1. I believe we addressed all your concerns — looking forward to see its usage in the wild :eyes:

pdanpdan commented 3 weeks ago

I still have the same concerns as before:

now the default HTML has an extra element most people don't need, and that does not fit the needs of most people that use teleport - so basically now everyone needs to define a bodyHtmlEnd()
I'm not very convinced of the HTML replacements done in onRenderClient (detailed below)

Both bodyHtml{Start,End} are concerned with shell HTML creation (that is mainly a one time job, outside of Vue app update cycle).

Main usages I can think of:

bodyHtmlStart - add analytics/tracking/captcha/garbage scripts - by replacing the content in onRenderClient funny things will happen on client side navigation
bodyHtmlEnd - add targets for teleport - I think at least one of the common usage is notification display. Now you perform an operation, display a notification and navigate on another page - the notification will break

Also this might generate problems in the context of nested routes

brillout commented 1 week ago

@pdanpdan Thanks for the feedback.

now the default HTML has an extra element most people don't need, and that does not fit the needs of most people that use teleport - so basically now everyone needs to define a bodyHtmlEnd()

Indeed, but I don't think it can cause any real issue?

On the flip side it enables this, which seems like a nice thing to have? Teleports work out of the box when using `<Teleport to="teleported">` and you don't have to use `bodyHtml{Begin,End}` then.

bodyHtmlStart - add analytics/tracking/captcha/garbage scripts - by replacing the content in onRenderClient funny things will happen on client side navigation

These are usually global and apply to all pages (i.e. they're defined at pages/+bodyHtmlStart.js), thus they won't get removed. I'm assuming that, in the following, a <script> tag won't be re-evaluated if it already existed before. I didn't test it, but I guess browsers are smart enough to only re-evaluate what changed.

document.body.innerHTML =
  document.body.innerHTML.substring(0, startIndex) + hookHtml + document.body.innerHTML.substring(endIndex)

But it's true that, if such analytics/tracking scripts are defined for only a subset of pages (e.g. pages/(marketing)/+bodyHtmlStart.js), then it's an issue. For example, if the user goes to a marketing page /about-us, then a non-marketing page/admin, then to the marketing page /about-us again: the analytics/tracking/... script is loaded twice in total, which is indeed an issue.

But not removing the bodyHtmlStart set by the previous page isn't a solution either. Because this would mean that we remove replaceAdjacentBodyHtml() altogether. Thus the marketing scripts are missing if the user goes to /admin then /about-us.

I'm not sure what a solution could be :thinking:

bodyHtmlEnd - add targets for teleport - I think at least one of the common usage is notification display. Now you perform an operation, display a notification and navigate on another page - the notification will break

replaceAdjacentBodyHtml() is called after changePage(). This means that, by the time the notification is removed, the new page is already rendered.

This question actually applies for config inheritance in general. When should the new config apply while transitioning from one page to another? I think the general answer is that the new config should apply after the new page is rendered. There has been a couple of back-and-forth about it, but so far I think this approach is the least error prone.

Also this might generate problems in the context of nested routes

From the perspective of config inheritance, there isn't any notion of nesting. For example, Nested Layouts are simply cumulative <Layout> components. (We could even support "nested" data fetching in the same way by making the data() hook cumulative, although it isn't the plan so far for DX reasons.)

So I don't think there is any additional issue here.

vikejs / vike-vue

Support for `teleports` with Vue SSR #87