🐛 BUG: passing children via props does not work properly on React components

[mihkeleidast]

What package manager are you using?

npm

What operating system are you using?

WSL/Ubuntu

Describe the Bug

React allows passing children in various ways, some examples:

<div className="App">
      <h3>direct children</h3>
      <Foo children="prop children" />
      <Foo children={<div>JSX children</div>} />
      <Foo>content children</Foo>
      <Foo children="prop children">content children</Foo>
      <h3>deep children</h3>
      <Bar items={[{ children: "first" }, { children: "second" }]} />
      <Bar
        items={[
          { children: <div>JSX children</div> },
          { children: <div>JSX children</div> }
        ]}
      />
</div>

Some of these do not render at all in Astro, and others that do render do not produce the same output as in pure React.

Here's a CodeSandbox that shows how these render in pure React: https://codesandbox.io/s/loving-wing-7vtk4?file=/src/App.js

Steps to Reproduce

1. Open and start the repro
2. See that the same code that works in pure React does not work in Astro.

Link to Minimal Reproducible Example (Optional)

https://github.com/mihkeleidast/astro-jsx-children-demo

[matthewp]

I went as far as to implement a fix for this but I now realize that we don't support this.

Remember that a .astro file is not a React component. Even though it is JSX-like, it's not meant to replicate the behavior of React's renderer.

The way to pass children in a .astro file is via the content, not props. Once you're in a React component you pass things the React way.

[mihkeleidast]

We've been talking about children specifically, but it's not just children/content at issue here. React allows the developer to pass children-like content to any prop, that can also be an array / sub-object etc.

Where I ran into issues specifically:
I have Tabs/TabsItem components. The common usage for those is:

<Tabs>
  <TabsItem>
    some content
  </TabsItem>
  <TabsItem>
    some content
  </TabsItem>
</Tabs>

It's important to note that in my case, Tabs/TabsItem communicate through a React.Context to provide the current tab info etc.

Now, in an Astro file, I tried to use them, in a way that one of those TabsItems has a Prism component inside it. I marked all the components with client:load, but since Astro hydrates them as islands, they don't share the React Context.

// index.astro
<Tabs client:load>
  <TabsItem client:load>
    some content
  </TabsItem>
  <TabsItem client:load>
    <Prism code={someCode} />
  </TabsItem>
</Tabs>

In order to make the context work, I created a React component that takes all the data in as props:

// TabsWrapper.jsx
import React from 'react';

import { Tabs, TabsItem } from '../../patterns/index.ts';

const TabsWrapper = (props) => {
    const { items } = props;

    return (
        <Tabs>
            {items.map((i) => {
                return (
                    <TabsItem {...i} key={i.id} />
                );
            })}
        </Tabs>
    )
}

export default TabsWrapper;

Then, in my index.astro:

<TabsWrapper
    client:load
    items={[
        {
            children: 'first content',
        },
        {
            children: 'second content',
        }
    ]}
/>

and this works! The components share the context, so tab switching works and all. However, as I said above, the content of the TabsItems could be anything, and I want to pass a Prism instance into one of those:

<TabsWrapper
    client:load
    items={[
        {
            children: 'content',
        },
        {
            children: <Prism code={code} />,
        }
    ]}
/>

This does not render at all unfortunately.

So, to reiterate - using React components that want to share some state through React Context API and still accept HTML/JSX/Astro content as props is not supported?

[eyelidlessness]

I agree with @mihkeleidast that this should be supported. I think it’s probably a good idea to handle this more generally by just allowing Astro elements to resolve inside-out by implicitly awaiting each render in any sub-expression before rendering the outer expression, which is already the case with “content”/children.

The fact that Astro renders are async-per-element is an implementation detail, and one most people wouldn’t expect coming from other component libraries. Pushing this limitation on users is probably going to continue to be a footgun.

I'm happy to look into this when I have some time, probably this weekend, if the Astro team doesn’t want to take it on but would be open to a contribution.

[matthewp]

I think this is a fine addition if applied generally. Want to keep in mind that:

• Passing promises as props is valid, so a solution here needs to be specific to h() promises.
• This is not React/Preact specific and children is irrelevant.

Will reopen with the understanding that this is about any props. I'll let someone else take this.

[eyelidlessness]

Will reopen with the understanding that this is about any props. I'll let someone else take this.

Maybe overly pedantic, but to avoid another round of clarification in case someone else picks this up before I do: it’s not even necessarily just about props, it’s about any Astro element/h call in any non-children position. That includes elements nested in an object passed as a prop, and likely elements referenced in frontmatter scripts.

[matthewp]

I think that's going a bit too far. I don't want to walk a potentially deep nested object looking for h() promises. This is already an edge case, let's not destroy perf in the pursuit of correctness.

Frontmatter scripts aren't a problem, if those are passed into Astro content it will be resolved already, if passed into a child component as props then fixing props takes care of that.

[eyelidlessness]

I think that's going a bit too far. I don't want to walk a potentially deep nested object looking for h() promises. This is already an edge case, let's not destroy perf in the pursuit of correctness.

Okay, this seems like a needless overreaction to a sincere interest in helping. It’s not an edge case, it’s extraordinarily common to use component elements as values in expressions. This use case is what prompted my clarification. It wouldn’t be addressed by resolving props directly.

I don’t think there’s a performance penalty for handling expressions generally rather than special casing them at the immediate prop level. In both cases you need to know there’s an inner expression that needs to be resolved by Astro, in both cases the existing call stack is already well suited to resolving efficiently. It’s probably more efficient to Promise.all than to special case top-level props.

But I do think there’s a persistent maintenance problem for Astro in using semantics in uncanny valley ways. If you want to fork JavaScript, by all means go for it. But it’s probably not gonna be a happy outcome for anyone.

[eyelidlessness]

To clarify, there’s no need to walk anything. Expressions are already going to evaluate from the inside out. You just need h to return a distinct subclass of Promise that parent Astro components can await.

[matthewp]

We would need to walk all objects passed as props to find h() derived promises.

<SomeComponent prop={{ nested: { another: { level: <div></div> } } }} />

We would need to walk this object to find the nested promise. Passing objects as props is common, so we'd be needlessly walking objects on the off-chance that they are doing this. Objects can even be recursive, so you have to account for objects you've already visited. If you've ever written a "deep clone" before, this is a lot like.

I don't think we should support this. If people really need the HTML string they can await it. There's an argument to made that trying to hide that Astro vdom is Promise-wrapped is too much.

[natemoo-re]

We don't have an answer for this just yet, but the original use case is totally valid. We will take some time to consider how one should approach this problem in Astro.

For some context:

• Astro's h function uses async functions that return a string of HTML
• React's createElement uses sync functions that return a VDOM object

We can try to paper over the differences (with our compiler or with the renderer interface), but at the end of the day, there will be places where this fact inevitably surfaces. Figuring out how to hide this isn't a major priority at the moment.

[natemoo-re]

@eyelidlessness If supporting this pattern is really straight-forward, we can support it, but I suspect there's a lot of yet-to-be-discovered complexity here.

I appreciate that you've been very upfront with your concerns about the astro format and DSLs other than JSX. We wouldn't be going down this road if we didn't think it was the right direction for our project, all trade-offs considered.

[eyelidlessness]

In broad strokes, this is how I’d go about it:

1. Subclass Promise to allow:
   • direct property access to its resolved value once resolved (either via Proxy or by iterating over only top-level keys)
   • outer promises to await outstanding inner promises in the call stack (again, this doesn’t require walking a tree, JS evaluates expressions inside-out)
2. Wrap props in that subclass and await Promise.all([ props, children ])
3. There is no step 3

This shouldn’t have much performance impact at all, beyond the inherent complexity of real world component trees.

[mihkeleidast]

BTW this almost works:

const template = await (<Prism code={code} />);

<TabsWrapper
    client:load
    items={[
        {
            children: 'content',
        },
        {
            children: template,
        }
    ]}
/>

As in, that renders and does not error out! But the awaited template is an HTML string, so in the browser this results as:

I would be totally fine if that was the necessary workaround, e.g. having to await the Astro component/template, but maybe there could be a way to pass it to React properly for rendering?

[mihkeleidast]

And the next thing I tried in my TabsWrapper was:

{items.map((i) => {
    return (
        <TabsItem {...i} key={i.id}>
            <div dangerouslySetInnerHTML={{ __html: i.children }} />
        </TabsItem>
    );
})}

and this works wonderfully!

Maybe this would just need to be documented as a valid usecase? I think necessary workarounds are fine and this seems like a necessary one, if the alternative is some kind of perf regression.

[mihkeleidast]

Okay taking my words back about this working "wonderfully". It worked partially in an Astro component inside a map:

---
const { variants } = Astro.props;
---
<div class="text">
    {variants?.length && variants.map(async (variant) => {
        const template = await (
            <>
                <Prism code={variant.content} lang={variant.editorMode} />
            </>
        );

        return (
            <TabsWrapper
                client:load
                items={[
                    {
                        id: `browser-${variant.id}-template`,
                        label: 'Template',
                        children: template,
                    },
                    {
                        id: `browser-${variant.id}-second`,
                        label: 'Second',
                        children: 'second content',
                    }
                ]}
            />
        )
    })}
</div>

Where it runs into issues if the template has a component with client:load in it:

const template = await (
    <>
        <ReactCounter client:load />
        <Prism code={variant.content} lang={variant.editorMode} />
    </>
);

This partially works as it renders but some of the HTML ends up duplicated etc, but the client-side hydration is partially broken, there's Uncaught SyntaxError: Invalid or unexpected token in the browser console.

As a separate Astro component, this does not work, but maybe it could?

---
import { Markdown, Prism } from 'astro/components';
import TabsWrapper from './TabsWrapper.jsx';

const { variant } = Astro.props;

const template = await (
    <Prism code={variant.content} lang={variant.editorMode} />
);

---

<div class="text">
    <TabsWrapper
        client:load
        items={[
            {
                id: `browser-${variant.id}-template`,
                label: 'Template',
                children: template,
            },
        ]}
    />
</div>

This fails with TypeError: tag.toLowerCase is not a function at _h (/_snowpack/pkg/astro.dist.internal.h.v0.17.2.js:6:11)

[juanpprieto]

Thanks for the discussion guys. I found it super useful in understanding where things are at the moment.

I arrived here after struggling to implement a simple React context provider inside a Layout component (in an attempt to understand) how global state could work inside astro.

I personally could not find a way to get a provider/consumer to work so wondering what is the plan/strategy/vision for working with global/shared state. By global, I'm thinking i.e user/account state or even a simpler global state for say working with modals and sidebars toggling etc.

Thanks for the amazing work so far. It's been fun testing things out.

[matthewp]

@eyelidlessness I don't think that algorithm will cover nested content like in this example:

<SomeComponent prop={{ nested: { another: { level: <div></div> } } }} />

Upon reflection this problem is deeper than Astro HTML being promised based. The promise part is an annoyance, but at its heart this issue is about framework interop. Astro HTML, even when resolved, is not a React component. It's an HTML string. So a React user (or Svelte user, etc) can't just use markup that's passed to it from props. That markup has to be converted to a framework-specific node type. That's what you see happening with children in the React renderer, for example. It converts the HTML string into a React component (the StaticHtml component) that the user's component can then inject where it wants.

So a solution to this problem needs to account for this issue (and probably needs coordination with renderers, some how).

[matthewp]

Would a shallow resolution satisfy the desire of this issue? As I talked about above, this seems hard/impossible to solve when passing into an object.

Also this requires renderer cooperation to turn the HTML strings into framework components.

If a shallow solution is acceptable I'd be willing to take the issue on.

[mihkeleidast]

Could there maybe be a utility component / function the user could manually wrap the contents with? Or would that also be as hard to implement as a completely automatic resolution? What I mean is, manually awaiting a nested content almost worked per my examples above, but there was an issue with nested client:load directives.

[mihkeleidast]

Investigated the nested client:load issue a bit. Seems like when there's a </script> inside a string in an actual <script></script> tag, browser interpret the one in the string as the ending tag for the actual script tag, breaking things.

See https://codepen.io/risker/pen/dyWqxLW?editors=1000 for a very simple reproduction.
It can be worked around by escaping the / in the string: https://codepen.io/risker/pen/OJmBLPo?editors=1000

Anyone know if this would be difficult to escape in Astro?

---

If that issue is fixed, I think the rest of this issue has workarounds - awaiting Astro components to pass them into framework component props is fine in my opinion. Maybe the StaticHTML component could be exported from the renderers so it could be used for dangerouslySetInnerHTML purposes? But it's not too difficult to add the same component to my project as well.

[matthewp]

I think there's a few good ideas here. I'll summarize what's been discussed in this thread:

• Desire to turn props into React components.
• A few solutions thrown out:
  • Subclassing Promises to detect which component renders need to be awaited.
  • awaiting all component calls in the compiler.
  • Shallow vs deep walking of props (to await subclassed Promise).
• Discussion about whether passing the HTML string is sufficient or not, as most frameworks need their own vnodes.

Since this change is non-trivial, and the solution is unclear, I think this needs to be resolved via the RFC process. The use-case is pretty clear but the design is not. Going to close this bug.

[pReya]

@matthewp @mihkeleidast You closed this ticket, because it was supposed to be solved in the RFC process. However, the linked RFCs (#751 and from there #877) are not very helpful for me and are already closed.

Can someone summarize the status of the described use case in #681 (comment)? Is this now possible somehow? Or is this being worked on? Are there any OPEN PRs/Discussions around this? Would be nice to have an update for all the "users" of Astro finding this Issue via Google, who are not Astro maintainers/devs.

[eigood]

Those linked RFCs are not at all about this issue; imho this issue should not be closed until the replacement discussion/RFC has been created. Since it's been a year since the original closure, and I couldn't actually find any RFC about this, it seems this problem has been dropped.

[cihad]

Any improvements? We like React because it is flexible. We want the same for Astro. Please do not ignore the requests of the community.