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

Improve Astro JSX rendering #10473

Merged
merged 4 commits into from
Mar 21, 2024
Merged

Improve Astro JSX rendering #10473

merged 4 commits into from
Mar 21, 2024

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Mar 18, 2024

Changes

Fixes #10158


This fix requires some context, so here it is 😄:

When rendering JSX nodes, there's currently a problem where it can call renderComponentToString to render React/Vue/MDX component from JSX nodes, but it can also render JSX nodes again if it hits the astro:jsx renderer (for MDX). Creating a loop.

Before

The solution to this problem is using "Skips" to make sure this order is ran (Worst case):

  1. Try to render JSX nodes.
  2. If fail, try to render components with React/Vue/MDX renderers.
  3. Detected MDX, try to render its JSX nodes again.
  4. Try to render components with React/Vue/MDX renderers.
  5. Detected MDX, try to render its JSX nodes again.
  6. Try to render JSX nodes again.
  7. Finished. Returns rendered string or error.

(I don't know why no4 and no5 repeated. Perhaps a typo in the logic?)

After

We use a single Symbol only to check if it had tried to render with React/Vue/MDX renderers. New order (Worst case):

  1. Try to render components with React/Vue/MDX renderers.
  2. Detected MDX, try to render its JSX nodes.
  3. Finished. Returns rendered string or error.

This refactor allows us to simplify the skips logic, and remove the console filter as we no longer unconditionally render vnode.type. renderComponentToString will check for it first.


Testing

Existing tests should pass. Also tested the repro manually from #10158 (didn't add test because Svelte 5 is still experimental and hard to setup test). Also tested repro from #4894.

Docs

n/a. bug fix.

Copy link

changeset-bot bot commented Mar 18, 2024

🦋 Changeset detected

Latest commit: 08e9949

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

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Mar 18, 2024
@bluwy bluwy marked this pull request as draft March 18, 2024 16:15
if (nonAstroPageNeedsHeadInjection(Component)) {
if (isPage && !result.partial && nonAstroPageNeedsHeadInjection(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.

We now only try to render the head if this component is a page and is non-partial. Otherwise there's chance we could render a component but not use it (with how the new renderJSX flow works)

@bluwy bluwy marked this pull request as ready for review March 20, 2024 16:29
@bluwy
Copy link
Member Author

bluwy commented Mar 21, 2024

!bench render

EDIT: It doesn't look like there's much gain

This comment was marked as resolved.

@ematipico
Copy link
Member

!bench render

EDIT: It doesn't look like there's much gain

Performance-wise, maybe not, but memory-wise, maybe yes. We don't construct a Skip class anymore, which is definitely some good nonetheless.

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Less code is the best code! 💪

Copy link
Member

@Princesseuh Princesseuh left a comment

Choose a reason for hiding this comment

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

Looks fine to me!

@bluwy bluwy merged commit 627e47d into main Mar 21, 2024
13 checks passed
@bluwy bluwy deleted the fix-jsx-render branch March 21, 2024 15:25
@astrobot-houston astrobot-houston mentioned this pull request Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Astro + Svelte 5 + MDX - Shows "undefined" instead of component
3 participants