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

Fixed component rerendering if there's only one renderer (solid-js) #12131

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

elite174
Copy link

@elite174 elite174 commented Oct 6, 2024

Changes

In this PR multiple server rerendering of the solid component is skipped if there's only one renderer in the project (solid-js).

I used only part of the solution for this issue.

If there're multiple rerenders, it should be processed in another way I suppose, but this little change can help if there's only one solid-js renderer.

Testing

Test was also added.

Docs

No need to update docs

Copy link

changeset-bot bot commented Oct 6, 2024

⚠️ No Changeset found

Latest commit: d2851c9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Oct 6, 2024
@elite174 elite174 force-pushed the fix-multiple-component-rerenders branch from 993f55b to d9fba90 Compare October 6, 2024 06:39
@elite174 elite174 changed the title Fix component rerenders if there's only one renderer Fix component rerendering if there's only one renderer Oct 6, 2024
@elite174 elite174 changed the title Fix component rerendering if there's only one renderer Fix component rerendering if there's only one renderer (solid-js) Oct 6, 2024
Comment on lines +131 to +135
// If there's only one renderer in the project (solid-js)
// we can skip the `check` calls and use that renderer (to not render solid component twice)
if (renderers.length === 1 && renderers[0].name === '@astrojs/solid') {
renderer = renderers[0];
} else {
Copy link
Author

Choose a reason for hiding this comment

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

this is the only change

@elite174 elite174 changed the title Fix component rerendering if there's only one renderer (solid-js) Fixed component rerendering if there's only one renderer (solid-js) Oct 6, 2024
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.

oops, accidentally approved.

break;
// If there's only one renderer in the project (solid-js)
// we can skip the `check` calls and use that renderer (to not render solid component twice)
if (renderers.length === 1 && renderers[0].name === '@astrojs/solid') {
Copy link
Contributor

@matthewp matthewp Oct 7, 2024

Choose a reason for hiding this comment

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

Why only Solid.js?

Copy link
Author

@elite174 elite174 Oct 7, 2024

Choose a reason for hiding this comment

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

because for some reason the tests fail for other frameworks. Moreover for solid it's critical to be rendered once, because solid doesn't have this "rerender" paradigm as react, preact or others.

More common solution could require much more efforts, so it should be carefully implemented. This is really easy fix which can help many people (because solid-start it's not that production-ready yet, so many people use astro+solid to get SSR), so we can do improvements step by step.

Copy link
Author

Choose a reason for hiding this comment

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

I tried to apply the solution suggested here, but looks like it doesn't work anymore for all the frameworks.

Copy link
Contributor

Choose a reason for hiding this comment

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

What tests fail? It's likely something we can fix. I don't think we should special-case one particular renderer in this case. There's nothing about this suggestion that shouldn't work everywhere.

If you could post the errors that occur when this is enabled for all renderers that would be helpful.

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.

2 participants