@astrojs/solid-js renders solid-js components twice

[btakita]

Astro Info

Astro                    v3.0.6
Node                     v18.17.0
System                   Linux (x64)
Package Manager          unknown
Output                   server
Adapter                  @astrojs/node
Integrations             @astrojs/solid-js

If this issue only occurs in one browser, which browser is a problem?

All

Describe the Bug

The astro request renders the solid-js components 2 times.

https://github.com/withastro/astro/blob/0ce0720c7f2c7ba21dddfea0b75d1e9b39c6a274/packages/astro/src/runtime/server/render/component.ts#L132

https://github.com/withastro/astro/blob/0ce0720c7f2c7ba21dddfea0b75d1e9b39c6a274/packages/astro/src/runtime/server/render/component.ts#L217

What's the expected result?

The astro request should render the solid-js components 1 time. Perhaps the result of r.ssr.check.call could be cached to be used again in enderer.ssr.renderToStaticMarkup.call

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-za4fie?file=package.json,src%2Fcomponents%2FC.tsx,src%2Fpages%2Findex.astro&on=stackblitz

Participation

• [ ] I am willing to submit a pull request for this issue.

[natemoo-re]

Unfortunately this is because we can't detect if a given function is a Solid component until we run it. Our React and Preact integrations do the same thing.

If you'd like to submit a PR to add a caching strategy, feel free!

[matthewp]

Closing since there is no claim that this breaks something, only that is non-ideal. Happy to receive a PR improving this though.

[forloopy]

@matthewp @natemoo-re @btakita

I have a relatively straightforward solution for this, working in my projects.

Using Astro 3.4.2, I have updated the following file:

astro/dist/runtime/server/render/component.js

Replacing these lines [97 - 112]:

if (!renderer) {
    let error;
    for (const r of renderers) {
        try {
        if (await r.ssr.check.call({ result }, Component, props, children)) {
                renderer = r;
           break;
        }
        } catch (e) {
            error ??= e;
    }
    }
    if (!renderer && error) {
        throw error;
    }
}

...with the following:

if (!renderer) {
    let error;
    try {
        // Single renderer
        if (Object.keys(renderers).length === 1) {
            renderer = renderers[0];
        }
        // Multiple renderers
        else {
            // Look for an override variable in the jsx/tsx component function, using the format, e.g. const renderer = 'solid-js'
            var regex = /(const|let|var)+[\s{1}]+(renderer)+[\s{1}]+[={1}]+[\s{1}]+(['"]{1})+(?<name>react|preact|solid-js|vue)+(['"]{1})/gm
            var matches = Array.from(Component.toString().matchAll(regex));
            if (!matches.length) {
                // No override, use first available renderer
                renderer = renderers[0];
            }
            else {
                // Use override renderer, if available
                for (const r of renderers) {
                    if (matches[0].groups.name === r.name) {
                        renderer = r;
                        break;
                    }
                }
            }
        }
    } catch (e) {
        error ??= e;
    }
    if (!renderer && error) {
        throw error;
    }
}

This approach removes the need for having to call a component twice, as it's relatively easy to determine which framework should be used, without having to use r.ssr.check.call(...).

It would seem logical that if only one framework has been added to the Astro project, e.g. Solid JS, then this should be the framework being used by default. Alternatively, if multiple frameworks have been added to the project, there needs to be an easy way to override the default framework.

This solution uses a simple variable in the component, to set a preferred renderer, e.g. const renderer = 'react';

Here's an example function with an override variable:

function MyComponent( props: any ) {
    const renderer = 'react';    
    return (
        <div>
            <span>This component will be rendered using React</span>        
        </div>
    );
}

If you think this is worth a PR, please let me know and I'll happily go ahead.

[PeterDraex]

@forloopy I like that there's no check for single renderer

[btakita]

Closing since there is no claim that this breaks something, only that is non-ideal. Happy to receive a PR improving this though.

@matthewp It actually made creating inline citations & rendering the citations as a footnote way more difficult than it should have been. It's one of the reasons why I no longer use Astro & wrote my own MPA stack. I think this should be taken seriously. It may not affect many devs with simpler requirements, but it's good to have system that works as expected in case someone wants to extend it. Meta note: having to jump through extra hoops to report an architectural issue that is difficult to reproduce is not developer-friendly. It's a trade off & I can see why you require a repro case...but making devs to more work to report an issue takes time & effort that could be spent on describing the issue in more detail + finding workarounds.

@forloopy Thank you for the solution. If I were still using Astro I would use it.

[Eworonuk]

@forloopy I would be happy if you made that PR! This has been a big annoyance for me in my project. The regex is a fine approach, or at minimum the check for a single renderer would probably cover a lot of use cases.

[elite174]

Hey guys, I made a PR for the case when there's only one renderer. I suppose that for multiple renderers it should be handled somehow differently (maybe), but at lease for one renderer we could do better.

https://github.com/withastro/astro/pull/12131

@Eworonuk @natemoo-re @matthewp

Thank you @forloopy for your solution.