-
Notifications
You must be signed in to change notification settings - Fork 35
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
Conversation
🦋 Changeset detectedLatest commit: a74d7da The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
packages/sku/sku-types.d.ts
Outdated
@@ -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>; |
There was a problem hiding this comment.
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:
renderToString: (element: ReactNode) => Promise<string>; | |
EXPERIMENTAL_renderToString: (element: ReactNode) => Promise<string>; |
There was a problem hiding this comment.
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.
Co-authored-by: Adam Skoufis <[email protected]>
Co-authored-by: Adam Skoufis <[email protected]>
Co-authored-by: Adam Skoufis <[email protected]>
Co-authored-by: Adam Skoufis <[email protected]>
Co-authored-by: Adam Skoufis <[email protected]>
f6f6cb6
to
f3ed34d
Compare
renderToString
parameter for renderApp
renderToStringAsync
parameter for renderApp
Reasoning/Details in ChangeSet
Testing