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

Add experimental renderToStringAsync parameter for renderApp #1050

Merged
merged 30 commits into from
Sep 20, 2024

Conversation

jahredhope
Copy link
Member

Reasoning/Details in ChangeSet

  • Added TanStack's react-query to a new fixture, to more genuinely recreate a suspended component.
    • Unfortunately, I only found out towards the end it embeds the the date in the dehydrated state, so I put in a hack to set the date to SEEK's 100th anniversary (If the fixture is still in then, please update it).
  • With some dependency management there were some unrelated fixture snapshot updates. I think the ignore file changes were actually always in there.

Testing

  • Render doesn't fail, even with a suspended component in new suspense fixture.
  • When testing using the fixture locally the 'loading todos' console message only appears on the server, not on the client. Unless you unmount/mount the component after render, in which case it'll be called on the client.

@jahredhope jahredhope requested a review from a team as a code owner September 15, 2024 23:30
Copy link

changeset-bot bot commented Sep 15, 2024

🦋 Changeset detected

Latest commit: a74d7da

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
sku Minor

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

@@ -42,6 +42,7 @@ interface SharedRenderProps {
interface RenderAppProps extends SharedRenderProps {
SkuProvider: ({ children }: { children: ReactNode }) => JSX.Element;
_addChunk: (chunkName: string) => void;
renderToString: (element: ReactNode) => Promise<string>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Thoughts on naming this something like:

Suggested change
renderToString: (element: ReactNode) => Promise<string>;
EXPERIMENTAL_renderToString: (element: ReactNode) => Promise<string>;

Copy link
Member Author

Choose a reason for hiding this comment

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

After some debate I'm now recommending calling it renderToStringAsync, this is:

  • Better Visibility: Easily searchable in code-bases
  • Better Migration: It's quite clear to consumers they now need to await the result
  • Maybe a bit uglier: But happy to trade off for the above benefits

Whilst the support for this is experimental, it's not unreasonable to be taken to production with care. I wouldn't want to set a precedence where teams send things called EXPERIMENTAL_ to production.

.changeset/sharp-berries-report.md Outdated Show resolved Hide resolved
docs/docs/static-rendering.md Outdated Show resolved Hide resolved
docs/docs/static-rendering.md Outdated Show resolved Hide resolved
docs/docs/static-rendering.md Outdated Show resolved Hide resolved
docs/docs/static-rendering.md Outdated Show resolved Hide resolved
packages/sku/entry/render/render-to-string.js Outdated Show resolved Hide resolved
jahredhope and others added 2 commits September 16, 2024 13:22
Co-authored-by: Adam Skoufis <[email protected]>
Co-authored-by: Adam Skoufis <[email protected]>
@jahredhope jahredhope changed the title Add experimental renderToString parameter for renderApp Add experimental renderToStringAsync parameter for renderApp Sep 16, 2024
@jahredhope jahredhope merged commit 70efb19 into master Sep 20, 2024
4 checks passed
@jahredhope jahredhope deleted the static-suspense branch September 20, 2024 00:34
@seek-oss-ci seek-oss-ci mentioned this pull request Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants