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

feat: add preact-ssr-prepass #9524

Merged
merged 7 commits into from
Jan 4, 2024

Conversation

aleksandrjet
Copy link
Contributor

Changes

It is the double of MR#7569

I Added preact-ssr-prepass library to preact integraion. This library allow to use lazy components in preact code. Without this i get unknown error in SSR.

While execute SSR preact-ssr-prepass will wait to load all dynamic imports so that preact-render-to-string can use they. More details about preact-ssr-prepass

Testing

Added examples/framework-preact-lazy and preact-lazy-component.test with lazy loading of component.

Docs

I don't think that it needs documentation, because i expected that Suspense and lazy functions will work.

It does not affect the current api, but it can slow build execution of preact components, because preact-ssr-prepass will add extra pass of tree. This pass will wait for all dynamic imports to be loaded, even if they don't exist in the code.

Reasons for the second attempt

I sent earlier MR#7569 with this fix. But this was declined by next reasons:

  • Marvin from Preact team said, that this functional will be included in the current library preact-render-to-string for server rendering by default. But a lot of time has passed since that answer. A few new versions (and major ones) of preact-render-to-string were realized, but they did not include work with lazy components. Using of preact-ssr-prepass is still recommended in official documentation

  • Also one of reasons was incorrect work with empty lazy components - Side effect of third argument of preact render #7868. While this was so, adding of preact-ssr-prepass would lead to some undocumented problems. But this problem was solved in astro@3. Empty lazy components now work correctly.

So, I think this MR has the right to a second try. It's been 6 months since the last discussion and lazy components still can't be rendered on the server.

Copy link

changeset-bot bot commented Dec 26, 2023

🦋 Changeset detected

Latest commit: 87c265a

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 pkg: example Related to an example package (scope) pkg: preact Related to Preact (scope) pkg: integration Related to any renderer integration (scope) pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review labels Dec 26, 2023
"@astrojs/preact": patch
---

Added preact-ssr-prepass library to the server code for rendering lazy components
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Added preact-ssr-prepass library to the server code for rendering lazy components
Allows rendering lazy components

@matthewp
Copy link
Contributor

Can you update to fix the build? Also, instead of adding a new example can you just update the current Preact example to use a lazy component?

@aleksandrjet aleksandrjet force-pushed the preact-prepass-render branch 3 times, most recently from a84842c to ed89a22 Compare December 28, 2023 05:02
@aleksandrjet
Copy link
Contributor Author

Can you update to fix the build? Also, instead of adding a new example can you just update the current Preact example to use a lazy component?

I fixed build and now it pass this stage and tests, but astro check was failed by other reason

@florian-lefebvre
Copy link
Member

@aleksandrjet please update your branch with the latest main, it will fix the erroring check

@aleksandrjet
Copy link
Contributor Author

aleksandrjet commented Dec 29, 2023

please update your branch with the latest main, it will fix the erroring check

thanks! all checks have now passed

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Some tiny nits, but the code changes lgtm! There's a merge conflict, but should be resolve-able by running pnpm i while resolving the conflicts. pnpm is able to resolve the conflicts automatically.

.changeset/cold-llamas-laugh.md Outdated Show resolved Hide resolved
examples/framework-preact/src/components/Counter.tsx Outdated Show resolved Hide resolved
@aleksandrjet aleksandrjet force-pushed the preact-prepass-render branch from c48073f to 3e200db Compare January 2, 2024 13:43
@aleksandrjet
Copy link
Contributor Author

should be resolve-able by running pnpm i

I got current changes and run pnpm i, it updated some packages

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Thanks! Looks great

@aleksandrjet
Copy link
Contributor Author

@florian-lefebvre @bluwy could I do anything else with this PR?

Copy link
Member

@florian-lefebvre florian-lefebvre left a comment

Choose a reason for hiding this comment

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

@sarah11918 I'd like your advice on the changeset. Also, do you think it's worth making a PR to the docs?

.changeset/blue-bobcats-remain.md Outdated Show resolved Hide resolved
.changeset/blue-bobcats-remain.md Outdated Show resolved Hide resolved
@aleksandrjet
Copy link
Contributor Author

@sarah11918 I'd like your advice on the changeset. Also, do you think it's worth making a PR to the docs?

I found last Sarah's comment in previous MR - #7569 (review). It said that we don't need to update preact docs

@ematipico ematipico merged commit 0903ef9 into withastro:main Jan 4, 2024
14 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Jan 4, 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) pkg: example Related to an example package (scope) pkg: integration Related to any renderer integration (scope) pkg: preact Related to Preact (scope) pr: docs A PR that includes documentation for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants