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

Renderer plugins #231

Merged
merged 44 commits into from
May 26, 2021
Merged

Renderer plugins #231

merged 44 commits into from
May 26, 2021

Conversation

natemoo-re
Copy link
Member

@natemoo-re natemoo-re commented May 24, 2021

Changes

Discussed Renderer Plugins (#155, #74) with @FredKSchott and we came up with a solution that vastly simplifies our render implementation and fixes a suite of other issues. This PR migrates our rendering implementation from static analysis performed during compilation to fully dynamic analysis performed at runtime.

Previously, we were using file extensions and user-supplied configuration to determine how to render any given component. This was problematic for the following reasons:

  • exports from NPM packages don't require file extensions
  • Users should be able to import components using any valid technique, for example from an index.js file (Support complex import cases other than default exports #189)
  • Statically analyzing the contents of every import would be prohibitively expensive
  • Rendering logic was not DRY and required astro to keep all frameworks as dependencies

Additionally, previous attempts at a generic renderer interface were quite complex and tightly coupled frameworks to their underlying JSX implementation.

By switching to a runtime analysis, we were able to

  • Remove significant complexity
  • Remove inconsistencies between frameworks (Svelte no longer requires special handling)
  • Provide a straightforward, extensible interface for component rendering
  • Support import using any technique (default, named, namespace, alias, etc)
  • Remove an expensive additional parse/compile step from static HTML => VNodes
  • Support mixed-framework component nesting. For example, embed a Svelte component inside of a Preact component. (Yes, really! I was also amazed 🤯)

Here is how straightforward our new renderer interface is, using Preact as an example.

// main entry point (index.js)
name: '@astro-renderer/preact',
  client: './client',
  server: './server',
// server entry point (./server.js)
import { h } from 'preact';
import { renderToString } from 'preact-render-to-string';
import StaticHtml from './static-html.js';

function check(Component, props) {
  try {
    return Boolean(renderToString(h(Component, props)));
  } catch (e) {}
  return false;
};

function renderToStaticMarkup(Component, props, children) {
  const html = renderToString(h(Component, props, h(StaticHtml, { value: children })))
  return { html };
}

export default { 
  check, 
  renderToStaticMarkup
}
// client entry point (client.js)
import { h, hydrate } from 'preact';
import StaticHtml from './static-html';

export default (element) => (Component, props, children) => hydrate(h(Component, props, h(StaticHtml, { value: children })), element);

Testing

  • Tests are passing
  • Tests updated where necessary

Docs

Docs will be tackled in a follow-up PR

  • Docs / READMEs updated
  • Code comments added where helpful

@changeset-bot
Copy link

changeset-bot bot commented May 24, 2021

🦋 Changeset detected

Latest commit: d530d90

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

This PR includes changesets to release 5 packages
Name Type
astro Minor
@astro-renderer/preact Minor
@astro-renderer/react Minor
@astro-renderer/svelte Minor
@astro-renderer/vue Minor

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

@natemoo-re natemoo-re mentioned this pull request May 24, 2021
12 tasks
@@ -16,6 +16,7 @@
},
"workspaces": [
"packages/*",
"packages/renderers/*",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we gain anything by publishing these separately (given that we are depending on them in astro anyways)?

Copy link
Member

Choose a reason for hiding this comment

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

For now, no. But I think it’s safer to have them be external. We might have optional renderers in the future that require a manual install, maybe. This lets us have that without changing the setup.

packages/astro/snowpack-plugin.cjs Outdated Show resolved Hide resolved

export interface CompileOptions {
logging: LogOptions;
resolvePackageUrl: (p: string) => Promise<string>;
astroConfig: AstroConfig;
extensions?: Record<string, ValidExtensionPlugins>;
Copy link
Member Author

Choose a reason for hiding this comment

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

No more extensions needed!

packages/astro/src/compiler/codegen/index.ts Show resolved Hide resolved
}
const importInfo = kind ? { componentUrl: getComponentUrl(), componentExport: getComponentExport() } : {};
return {
wrapper: `__astro_component(${name}, ${JSON.stringify({ hydrate: kind, displayName: name, ...importInfo })})`,
Copy link
Member Author

Choose a reason for hiding this comment

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

We now have a single entry point for all rendering and hydration logic: __astro_component

@@ -625,7 +501,7 @@ async function compileHtml(enterNode: TemplateNode, state: CodegenState, compile
paren++;
return;
}
const COMPONENT_NAME_SCANNER = /^[A-Z]/;
const COMPONENT_NAME_SCANNER = /^[A-Z]|[.]/;
Copy link
Member Author

Choose a reason for hiding this comment

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

Added support for JSX-style namespaced.components, for example Framer Motion's <motion.div>.

Copy link
Contributor

Choose a reason for hiding this comment

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

this could match bad element names ? like A.. we could change to match all Pascal?.Case names with /^[A-Z][a-z]+([.]?)(?:[A-Z][a-z]+)*/

Copy link
Member Author

Choose a reason for hiding this comment

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

It could, good catch. One case your RegExp doesn't catch is something like motion.div, which is valid for JSX.

Copy link
Contributor

Choose a reason for hiding this comment

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

/^[A-Z][a-z]+([.]?)(?:[A-Z][a-z]+)*|^[a-z]+[.]+[a-z]+/ starts looking horrendous

Copy link
Member

Choose a reason for hiding this comment

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

I smell a unit test needed 😄. Either that, or perhaps you can use something like moo that can take multiple, simpler RegExs and combine them together (with “states”)

Copy link
Contributor

Choose a reason for hiding this comment

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

if (Component == null) {
throw new Error(`Unable to render <${componentProps.displayName}> because it is "${Component}"!\nDid you forget to import the component or is it possible there is a typo?`);
}
const renderer = resolveRenderer(Component);
Copy link
Member Author

Choose a reason for hiding this comment

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

Let's resolve the renderer for this Component based on the injected __renderers (from Astro's Snowpack plugin).

Comment on lines 11 to 39
/** For a given component, resolve the renderer. Results are cached if this instance is encountered again */
function resolveRenderer(Component: any) {
let rendererName = rendererCache.has(Component) ? rendererCache.get(Component) : undefined;
let renderer = rendererName ? __renderers.find(({ name }: any) => name === rendererName) : undefined;

if (!rendererName) {
for (const __renderer of __renderers) {
const shouldUse = __renderer.check(Component)
if (shouldUse) {
rendererName = __renderer.name;
renderer = __renderer;
break;
}
}
}

if (!renderer) {
const name = typeof Component === 'function' ? (Component.displayName ?? Component.name) : `{ ${Object.keys(Component).join(', ')} }`;
throw new Error(`No renderer found for ${name}! Did you forget to add a "renderer" to your Astro config?`);
}

return renderer;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Not too complicated, just loop through the renderers and run check(Component) to see if this renderer should claim this component. check is usuallyy cheap for objects (easy to check the shape) but expensive for functions (they need to be rendered to check the shape), so we cache the results in a WeakMap.

Comment on lines 271 to 277
const defaultRenderers = [
'@astro-renderer/vue',
'@astro-renderer/svelte',
'@astro-renderer/preact',
'@astro-renderer/react'
];
Copy link
Member Author

Choose a reason for hiding this comment

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

Open to discussion here, what should Astro have out of the box? I'd prefer not having all these as direct dependencies but it does make for a great user experience.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should absolutely have all of these by default. "Everything is an extension" tooling is frustrating and one of the things that I think people are excited about with Astro, we give you sane defaults out of the box.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with Matthew—why not include everything? At least until it costs us something (performance, size, etc.)

Copy link
Contributor

Choose a reason for hiding this comment

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

great stuff and the ability to switch off if needed via renderers in astro.config.mjs is perfect

declare let __renderers: any[];

__rendererSources = ['', ...__rendererSources];
__renderers = [{ default: astro }, ...__renderers].map(renderer => typeof renderer.default === 'function' ? renderer.default() : renderer.default);
Copy link
Member Author

Choose a reason for hiding this comment

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

Resolve user-provided/injected renderers. The built-in Astro renderer cannot be configured (for obvious reasons).

packages/renderers/preact/static-html.js Outdated Show resolved Hide resolved
enter(node) {
if (!hasComponents) return;
switch (node.name) {
case 'head': {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not important for this issue but we really shouldn't be requiring <head> and <body> elements as those are not required in HTML. What we should be doing in a case like this is if there is not a head, find the first non-head element (so anything that's not one of these: https://htmlhead.dev/#elements) and inject before that element. The browser will parse it as inside of the head. We could create a utility to make these easier for transformers to do.

Not critical for this issue as we have the requirement in a few spots but something to definitely cleanup later.

let end = typeof wrapperEnd === 'string' ? wrapperEnd : wrapperEnd({ roots: 'roots' });

const script = `<script type="module" data-astro-hydrate="${astroId}">
const roots = document.querySelectorAll('[data-astro-root="${astroId}"]');
Copy link
Contributor

Choose a reason for hiding this comment

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

Will there ever be more than 1 root with that id? What's the scenario?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, if you render any component multiple times with the same props!

@@ -0,0 +1,5 @@
import { h } from 'preact';

const StaticHtml = ({ value }) => value ? h('div', { 'data-astro-children': '', dangerouslySetInnerHTML: { __html: value }}) : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this always inserting a div inside of a preact component? Is that needed for server-only usage? Just for consistency?

Copy link
Member Author

Choose a reason for hiding this comment

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

Astro always passes children as a string of HTML. It's the renderer's job to convert that into something the framework can render. For P/React, you unfortunately need an element wrapper to use dangerouslySetInnerHTML: {{ __html }} (though I wish Fragment could support this).

This is actually what allowed us to completely decouple JSX from the renderers—previously we were converting the HTML back to framework-specific VNodes as needed, but that introduces a lot of overhead and IMO is not worth the cost.

@matthewp
Copy link
Contributor

The build also needs to be updated as it does some similar stuff surrounding finding which frameworks are used as codegen was doing previously. Look under src/build/page.ts.

Happy with the overall direction, very clean! There are still a few things to be cleaned up, would like to rereview when this is closer to being ready.

Comment on lines +35 to +38
<react.Counter:visible>
<h1>Hello React!</h1>
<p>What's up?</p>
</ReactCounter:load>
</react.Counter:visible>
Copy link
Contributor

@duncanhealy duncanhealy May 24, 2021

Choose a reason for hiding this comment

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

ReactCounter -> react.Counter will not match the COMPONENT_NAME_SCANNER regexp?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with your suggestion to use a more complete RegExp that checks for validity, too.

But this does actually match! /^[A-Z]|\./ runs as (^[A-Z] OR \.) => starts with a capital letter OR has a period

Copy link
Contributor

@duncanhealy duncanhealy left a comment

Choose a reason for hiding this comment

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

🚀

Comment on lines +88 to 91
const optimizers = [transformHydration(), transformStyles(opts), transformDoctype(opts), transformModuleScripts(opts), transformCodeBlocks(ast.module)];

for (const optimizer of optimizers) {
collectVisitors(optimizer, htmlVisitors, cssVisitors, finalizers);
Copy link
Contributor

Choose a reason for hiding this comment

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

wonder if this could be a source of race condition for css bundling for #230

Suggested change
const optimizers = [transformHydration(), transformStyles(opts), transformDoctype(opts), transformModuleScripts(opts), transformCodeBlocks(ast.module)];
for (const optimizer of optimizers) {
collectVisitors(optimizer, htmlVisitors, cssVisitors, finalizers);
const optimizers = [transformHydration(), transformStyles(opts), transformDoctype(opts), transformModuleScripts(opts), transformCodeBlocks(ast.module)];
for (const optimizer of optimizers) {
await collectVisitors(optimizer, htmlVisitors, cssVisitors, finalizers);

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, it might be? @drwpow might have more insight here—I think the goal is that all of these can execute in parallel. In this case, the transformHydration optimizer is injecting a <style> tag to set display: contents for [data-astro-root] and [data-astro-children].

Copy link
Member

@drwpow drwpow May 24, 2021

Choose a reason for hiding this comment

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

I think #230 was unrelated, but that’s a good callout—this possibly could be hiding a race condition. I’m not positive, but I think these might be collected in a deterministic order, even if they resolve/complete in random orders, so even though this is parallelized this may yield the same results each time.

But either way will keep an eye on it (also we added a test for stylesheet ordering in #232 so that should alert us if stylesheet bundling order is still flaky.)

Copy link
Contributor

Choose a reason for hiding this comment

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

collectVisitors isn't async so I'm not sure why @duncanhealy suggested adding an await to it.

Copy link
Contributor

Choose a reason for hiding this comment

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

the finalizers array of async functions are being pushed using the array ref passed in - was wondering if something was being called by the event loop out of sync or garbage collector interfering. The code suggestion was to call attention to the line. There are a few ways it could be refactored if it is causing the race condition. ?? finalizers.push(collectVisitors(optimizer, htmlVisitors, cssVisitors) where collectVisitors returned => transformer.finalize ?

Copy link
Member Author

@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.

Leaving some notes on what has changed since last time.

const StaticHtml = ({ value }) => {
if (!value) return null;
return h('astro-fragment', { dangerouslySetInnerHTML: { __html: value }});
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Using an <astro-fragment> custom element rather than div. There are situations where <div> would be semantically invalid.

Copy link
Member Author

Choose a reason for hiding this comment

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

The <div data-astro-root> has also been replaced with <astro-root>. Neither custom element is defined right now, but we could use them in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since an astro-fragment is essentially a span here should we just use a span instead? <span data-astro-fragment>. I feel a little weird about using a custom element that we are not upgrading to a real custom element.

Copy link
Contributor

Choose a reason for hiding this comment

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

Custom elements should “transparent”, as in they can accept any children. But span is explicitly inline and restricts its children/risks DOM breakage, pretty much the inverse of the div problem. Another option is the is attribute, but that’s effectively the same as a custom element.

Comment on lines 1 to 72
type GetHydrateCallback = () => Promise<(element: Element, innerHTML: string|null) => void>;
type AstroSetupInfo = ReturnType<typeof setupAstro>;

/**
* For a given `astroId`, find every matching `astro-root` element.
*
* We'll also check the first match for an `astro-fragment` element
* and grab the `innerHTML` if we find it. We use this HTML string
* to pass the correct `children` back to the renderer.
*
* Note that every matching `astro-root` will have the same `innerHTML`
* because `astroId` is a hash based on the generated HTML.
*/
const setupAstro = (astroId: string) => {
const roots = document.querySelectorAll(`astro-root[uid="${astroId}"]`);
let innerHTML = null;
let children = roots[0].querySelector(`astro-fragment`);
if (children) innerHTML = children.innerHTML;
return { roots, innerHTML };
}

/**
* Execute hydration on every matching `astro-root` element.
* This is a shared utility for all hydration methods to run.
*/
const doHydrate = async ({ roots, innerHTML }: AstroSetupInfo, getHydrateCallback: GetHydrateCallback) => {
const hydrate = await getHydrateCallback();
for (const root of roots) {
hydrate(root, innerHTML);
}
}

/**
* Hydrate this component immediately
*/
export const onLoad = async (astroId: string, getHydrateCallback: GetHydrateCallback) => {
doHydrate(setupAstro(astroId), getHydrateCallback);
}

/**
* Hydrate this component as soon as the main thread is free
* (or after a short delay, if `requestIdleCallback`) isn't supported
*/
export const onIdle = (astroId: string, getHydrateCallback: GetHydrateCallback) => {
if ('requestIdleCallback' in window) {
(window as any).requestIdleCallback(() => doHydrate(setupAstro(astroId), getHydrateCallback))
} else {
setTimeout(() => doHydrate(setupAstro(astroId), getHydrateCallback), 200)
}
}

/**
* Hydrate this component when one of it's children becomes visible.
* We target the children because `astro-root` is set to `display: contents`
* which doesn't work with IntersectionObserver
*/
export const onVisible = async (astroId: string, getHydrateCallback: GetHydrateCallback) => {
const context = setupAstro(astroId);
const io = new IntersectionObserver(async ([entry]) => {
if (!entry.isIntersecting) return;
// As soon as we hydrate, disconnect this IntersectionObserver for every `astro-root`
io.disconnect();
doHydrate(context, getHydrateCallback);
});

for (const root of context.roots) {
for (let i = 0; i < root.children.length; i++) {
const child = root.children[i];
io.observe(child);
}
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Hydration logic is split out into a separate file rather than being inlined.

This cuts down on the final payload size and allows us to author the hydration handlers in JS (rather than a string).

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you confirm that these exports will be tree-shaken away of not used (I wouldn't want an app to pay for onVisible's larger JS if they aren't using it).

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. It does not tree-shake right now because this file is passed as an entry point. I'll see if there's a way to tweak that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's split this into multiple files in that case and just import the one that is needed.

import astro from './renderer-astro';

// A more robust version alternative to `JSON.stringify` that can handle most values
// see https://github.com/remcohaszing/estree-util-value-to-estree#readme
const serialize = (value: Value) => generate(valueToEstree(value));
Copy link
Member Author

Choose a reason for hiding this comment

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

As the comment says, this is a better alternative to JSON.stringify. Are there other places we should adopt this?

Ref: https://github.com/remcohaszing/estree-util-value-to-estree#readme

for (const root of roots) {
hydrate(root)(Component, ${props}, innerHTML);
}
${end}
Copy link
Member Author

@natemoo-re natemoo-re May 25, 2021

Choose a reason for hiding this comment

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

We're still inlining this script, but it's just passing a callback for /_astro_internal/hydrate.js to run.

Generated output looks like this:

import { onVisible } from '/_astro_internal/hydrate.js';
onVisible("Z2cdXDi", async () => {
  const [{ Counter: Component }, { default: hydrate }] = await Promise.all([import("/_astro/components/ReactCounter.js"), import("/_snowpack/link/packages/renderers/react/client.js")]);
  return (el, children) => hydrate(el)(Component, { "class": "astro-ldKR6c5M" }, children);
});

Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally I'd love to collect these scripts and move them to the end of the document/body. Any ideas on how we could accomplish that with our h function?

Copy link
Contributor

@duncanhealy duncanhealy May 25, 2021

Choose a reason for hiding this comment

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

something like > ? document.body.insertAdjacentHTML('afterend', scriptElementContents) or beforeend

Copy link
Member Author

Choose a reason for hiding this comment

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

These scripts are generated by Astro's h function on the server-side, so no DOM available. We could add one final post-render step to optimizes the final HTML, but I'm not sure how the team feels about that.

Copy link
Contributor

Choose a reason for hiding this comment

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

@natemoo-re I wouldn't want to move them to the end of the body, I'd want to move them up to the head so they can start fetching deps earlier, but that isn't easy given that some components are added dynamically.

@natemoo-re natemoo-re changed the title WIP: Renderer plugins Renderer plugins May 25, 2021
@@ -5,7 +5,8 @@ import type { LogOptions } from './logger';
import type { AstroConfig, CollectionResult, CollectionRSS, CreateCollection, Params, RuntimeMode } from './@types/astro';

import { existsSync } from 'fs';
import { fileURLToPath } from 'url';
import { fileURLToPath, pathToFileURL } from 'url';
import { posix as path } from 'path';
Copy link
Member

@drwpow drwpow May 26, 2021

Choose a reason for hiding this comment

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

Small suggestion: I’ve noticed that we all have different styles when it comes to expanding different methods for node:path. I’ve even seen us have local variables named path that cause us to remap node:path to something else entirely to avoid conflict. It’s just different in every file. Here, I think renaming posix as path may be a little unexpected here, especially if non-POSIX is needed.

What do you think about just standardizing import path from 'path' in the project everywhere? It’s a little more verbose but it’s predictable, and we don’t need to keep changing import statements every time we need a new method

Copy link
Member Author

Choose a reason for hiding this comment

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

That is a great point—that has been bothering me, too. I'll open an issue and address this in a followup PR.

Copy link
Member

@drwpow drwpow left a comment

Choose a reason for hiding this comment

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

Amazing! Left a couple comments, but no major feedback. Excited to merge this!

// If we're NOT hydrating this component, just return the HTML
if (!componentProps.hydrate) {
// It's safe to remove <astro-fragment>, static content doesn't need the wrapper
return html.replace(/\<\/?astro-fragment\>/g, '');
Copy link
Contributor

Choose a reason for hiding this comment

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

Not important at the moment but when we go with streaming we'll want to just prevent renderToStaticMarkup from adding the fragment thing, that way we can just stream the chunks and not wait for the full HTML.

Copy link
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

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

Want to talk about a couple of things here, most important I think is to split up the hydrate.ts, we should keep the JS minimal. If that means having some redundant code that's preferable to loading code that the client isn't using.

Comment on lines 1 to 72
type GetHydrateCallback = () => Promise<(element: Element, innerHTML: string|null) => void>;
type AstroSetupInfo = ReturnType<typeof setupAstro>;

/**
* For a given `astroId`, find every matching `astro-root` element.
*
* We'll also check the first match for an `astro-fragment` element
* and grab the `innerHTML` if we find it. We use this HTML string
* to pass the correct `children` back to the renderer.
*
* Note that every matching `astro-root` will have the same `innerHTML`
* because `astroId` is a hash based on the generated HTML.
*/
const setupAstro = (astroId: string) => {
const roots = document.querySelectorAll(`astro-root[uid="${astroId}"]`);
let innerHTML = null;
let children = roots[0].querySelector(`astro-fragment`);
if (children) innerHTML = children.innerHTML;
return { roots, innerHTML };
}

/**
* Execute hydration on every matching `astro-root` element.
* This is a shared utility for all hydration methods to run.
*/
const doHydrate = async ({ roots, innerHTML }: AstroSetupInfo, getHydrateCallback: GetHydrateCallback) => {
const hydrate = await getHydrateCallback();
for (const root of roots) {
hydrate(root, innerHTML);
}
}

/**
* Hydrate this component immediately
*/
export const onLoad = async (astroId: string, getHydrateCallback: GetHydrateCallback) => {
doHydrate(setupAstro(astroId), getHydrateCallback);
}

/**
* Hydrate this component as soon as the main thread is free
* (or after a short delay, if `requestIdleCallback`) isn't supported
*/
export const onIdle = (astroId: string, getHydrateCallback: GetHydrateCallback) => {
if ('requestIdleCallback' in window) {
(window as any).requestIdleCallback(() => doHydrate(setupAstro(astroId), getHydrateCallback))
} else {
setTimeout(() => doHydrate(setupAstro(astroId), getHydrateCallback), 200)
}
}

/**
* Hydrate this component when one of it's children becomes visible.
* We target the children because `astro-root` is set to `display: contents`
* which doesn't work with IntersectionObserver
*/
export const onVisible = async (astroId: string, getHydrateCallback: GetHydrateCallback) => {
const context = setupAstro(astroId);
const io = new IntersectionObserver(async ([entry]) => {
if (!entry.isIntersecting) return;
// As soon as we hydrate, disconnect this IntersectionObserver for every `astro-root`
io.disconnect();
doHydrate(context, getHydrateCallback);
});

for (const root of context.roots) {
for (let i = 0; i < root.children.length; i++) {
const child = root.children[i];
io.observe(child);
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's split this into multiple files in that case and just import the one that is needed.

packages/astro/src/runtime.ts Outdated Show resolved Hide resolved
packages/astro/src/runtime.ts Outdated Show resolved Hide resolved
const StaticHtml = ({ value }) => {
if (!value) return null;
return h('astro-fragment', { dangerouslySetInnerHTML: { __html: value }});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Since an astro-fragment is essentially a span here should we just use a span instead? <span data-astro-fragment>. I feel a little weird about using a custom element that we are not upgrading to a real custom element.

@matthewp
Copy link
Contributor

For some reason I can't comment on my astro-fragment comment. Wanted to say that I don't feel strongly about changing that. I do like astro-root, I wonder if we could (later) change this to something like: <astro-root on-idle component="/_astro/components/Card.js"> or something like that, getting rid of the inline JS entirely.

@natemoo-re
Copy link
Member Author

natemoo-re commented May 26, 2021

Cool, we're on the same page. These changes were to set us up to do that kind of thing in the future!

duncanhealy added a commit that referenced this pull request May 26, 2021
* documentation: post #231 merge renderers are a config option

* Update docs/config.md to reorder
@natemoo-re natemoo-re requested a review from matthewp May 26, 2021 17:31
@natemoo-re natemoo-re merged commit 643c880 into main May 26, 2021
@natemoo-re natemoo-re deleted the feat/renderer-plugins branch May 26, 2021 18:30
ewatch pushed a commit that referenced this pull request May 31, 2021
* documentation: post #231 merge renderers are a config option

* Update docs/config.md to reorder
ewatch pushed a commit that referenced this pull request May 31, 2021
* refactor: pluggable renderers

* refactor: cache renderer per component

* docs: update comments on snowpack plugin `transform` method

* docs: add comments to renderer plugins

* refactor: convert components to Map

* fix: pass children through to astro __render

* refactor: move Components/ComponentInfo to shared types

* refactor: remove `gatherRuntimes` step, just scan output for imports

* refactor: update isComponentTag logic

* chore: move dependencies to renderers

* fix: cross-platform transform injection

* feat: defer renderer to react, fallback to preact

* fix: use double quotes in generated script

* test: fix failing children tests

* test: add workspaceRoot to all tests

* fix: pass props to renderer check

* chore: add test:core script back for convenience

* chore: remove unused external

* chore: rename renderers

* chore: add astring, estree-util-value-to-estree

* chore: render-component => __astro_component

* refactor: split hydrate logic to own file

* refactor: use `astro-fragment` rather than `div`

* chore: remove unused hooks

* chore: delete unused file

* chore: add changesets

* fix: Astro renderer should be async

* fix: remove <astro-fragment> for static content

* test: fix failing test

* chore: normalize config interface

* feat: allow renderers to inject a snowpackPlugin

* fix: resolve import URL before using dynamic import

* refactor: update renderers to use separate /server entrypoint

* refactor: update server renderer interface

* fix: get renderers working again

* test: debug failing test

* test: better debug

* test: better debug

* test: remove debug

* fix: support esm and cjs packages via "resolve"

* refactor: split hydrate functions into individual files

* fix: dependency resolution relative to projectRoot

* fix: @snowpack/plugin-postcss needs to be hoisted

* fix: do not test prettier-plugin-astro as it's not ready for primetime
SiriousHunter pushed a commit to SiriousHunter/astro that referenced this pull request Feb 3, 2023
* refactor: pluggable renderers

* refactor: cache renderer per component

* docs: update comments on snowpack plugin `transform` method

* docs: add comments to renderer plugins

* refactor: convert components to Map

* fix: pass children through to astro __render

* refactor: move Components/ComponentInfo to shared types

* refactor: remove `gatherRuntimes` step, just scan output for imports

* refactor: update isComponentTag logic

* chore: move dependencies to renderers

* fix: cross-platform transform injection

* feat: defer renderer to react, fallback to preact

* fix: use double quotes in generated script

* test: fix failing children tests

* test: add workspaceRoot to all tests

* fix: pass props to renderer check

* chore: add test:core script back for convenience

* chore: remove unused external

* chore: rename renderers

* chore: add astring, estree-util-value-to-estree

* chore: render-component => __astro_component

* refactor: split hydrate logic to own file

* refactor: use `astro-fragment` rather than `div`

* chore: remove unused hooks

* chore: delete unused file

* chore: add changesets

* fix: Astro renderer should be async

* fix: remove <astro-fragment> for static content

* test: fix failing test

* chore: normalize config interface

* feat: allow renderers to inject a snowpackPlugin

* fix: resolve import URL before using dynamic import

* refactor: update renderers to use separate /server entrypoint

* refactor: update server renderer interface

* fix: get renderers working again

* test: debug failing test

* test: better debug

* test: better debug

* test: remove debug

* fix: support esm and cjs packages via "resolve"

* refactor: split hydrate functions into individual files

* fix: dependency resolution relative to projectRoot

* fix: @snowpack/plugin-postcss needs to be hoisted

* fix: do not test prettier-plugin-astro as it's not ready for primetime
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants