feat: identify different JSX frameworks during SSR#15700
feat: identify different JSX frameworks during SSR#15700ocavue wants to merge 3 commits intowithastro:mainfrom
Conversation
🦋 Changeset detectedLatest commit: a5bfde2 The changes in this PR will be included in the next version bump. 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 |
| for (const r of renderers) { | ||
| try { | ||
| if (await r.ssr.check.call({ result }, Component, props, children)) { | ||
| if (await r.ssr.check.call({ result }, Component, props, children, metadata)) { |
There was a problem hiding this comment.
This is the only change that I really need in astro package.
| // Attempt: use explicitly passed renderer name for custom renderers. This is put | ||
| // last to avoid potential conflicts with the previous implementations. | ||
| if (!renderer && metadata.hydrateArgs) { | ||
| const rendererName = metadata.hydrateArgs; | ||
| if (typeof rendererName === 'string') { | ||
| renderer = renderers.find(({ name }) => name === rendererName); | ||
| } | ||
| } |
There was a problem hiding this comment.
These changes are just for the new test case. They are harmless and even useful anyway.
|
Ok cool, so this is basically all handled inside of the integration renderers. This prevents us from having to run the code if we already know its outside of the directory scope, is that right? |
That's correct. |
|
@ocavue can you add this to the React integration as well? |
|
Just noting that since there are potential breaking changes here, this would need some kind of breaking change guidance: https://contribute.docs.astro.build/docs-for-code-changes/changesets/#breaking-changes It would probably be a good idea to include any of the framework integrations with breaking changes to this list, too: https://v6.docs.astro.build/en/guides/upgrade-to/v6/#official-astro-integrations (Right now, it's just the adapters, but if this affects e.g. something previously rendered as one JSX framework could now be rendered as another, we should have a |
Problem
When multiple JSX renderers are used (e.g. React + Preact), Astro doesn't really know which renderer it should use to render a
.jsxfile during SSR. Astro will try to guess the renderer on a best-effort basis, for example:QwikComponent, then it won't be treated as a React component (code link)<undefined>in the HTML, then it won't be treated as a Preact component (code link)These guesses are fragile and error-prone. For example, in #15341, a React component is rendered using
preactduring SSR. This could cause subtle bugs, such as hydration mismatches when usingpreactto render a React component. Currently we just patchconsole.errorand pretend it's not a problem.Idea
When multiple JSX renderers are used in the same project, users SHOULD specify the
include/excludepatterns to identify the components that should be rendered by each renderer. Otherwise a warning is already shown to the user.These
include/excludepatterns are currently only used by the Vite JSX transform plugins. We can reuse them during the SSR phase to pick the correct renderer for a JSX component.Changes
This PR includes three main changes:
In
packages/astro/src: I pass themetadatato therender.ssr.check()function. Notice that this matches the existing signature of thecheck()function.In
packages/astro/test: I build a comprehensive test fixture with two mock renderers (woof/meow) that demonstrates the filter pattern. Tests cover different scenarios: SSR-only components,client:loadcomponents, andclient:onlycomponents.In
packages/integrations/preact: I reuse theinclude/excludeoptions passed to the integration. During the SSR phase, these options are used to check the component path frommetadata.componentUrl.For now, I only update
@astrojs/preactin this PR because I want to keep the PR small and focused. But in an ideal world, we should update all the JSX renderers to use this approach, including official renderers like@astrojs/reactand third-party renderers like@qwikdev/astro.Potential breaking changes
Let's say we have a project with the following config:
And we have the following file structure:
Before this PR, all three components would be rendered by
preactduring SSR.After this PR,
Button.jsxwill be rendered bypreact,Box.jsxwill be rendered byreact, andTooltip.jswon't be rendered by any renderer, which will cause a build-time error.Alternative Approach
In my current approach, I let
@astrojs/preacthandle its owninclude/excludepatterns and decide whether to render a component by itself. In an alternative approach, we could let the Astro core handle theinclude/excludepatterns. I didn't choose this alternative approach becauseinclude/excludeare technically renderer-specific. Although almost all Vite plugins use these patterns since this pattern is from Rollup, a Vite plugin could prefer to use other patterns to identify if a.jsxfile is a valid component, especially in the future when Rolldown replaces Rollup as the default bundler.Known limitation
SSR-only components (those without any
client:*directive) don't havemetadata.componentUrl, because the Astro compiler only emitsclient:component-pathfor hydrated components. The filter can't apply in this case, andcheck()falls back to its existing try-render behavior. Therefore, my current PR only improves the JSX components withclient:*directive.I can submit another purpose to the compiler repo with more details, if the team thinks this is a good idea.
Docs
No ready.