-
Notifications
You must be signed in to change notification settings - Fork 391
feat(getServerState): allow users to inject renderToString #3658
Conversation
There are some cases where the combination of trying to make sure renderToString doesn't end up in a browser bundle, being runnable on esm + cjs, react 17 and 18, .js extension etc. blows up. One of those is pnpm caching removing "unused" packages. This PR introduces a new argument `renderToString` to `getServerState` so you can inject the dependency yourself, meaning the import is within your own code and won't be purged. ```js import { renderToString } from 'react-dom/server'; await getServerState(<App/>, renderToString); await getServerState(<App/>, import('react-dom/server').then(mod => mod.renderToString)); ``` fixes #3633 closes #3618 see vercel/next.js#40067
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 8a21a6f:
|
✅ Deploy Preview for react-instantsearch ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
Don't we want to be more cautious and have it in a param object? await getServerState(<App/>, { renderToString }); |
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.
Neat idea!
import type { | ||
InstantSearchServerContextApi, | ||
InstantSearchServerState, | ||
} from 'react-instantsearch-hooks'; | ||
|
||
type SearchRef = { current: InstantSearch | undefined }; | ||
|
||
export type RenderToString = (element: JSX.Element) => unknown; |
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.
export type RenderToString = (element: JSX.Element) => unknown; | |
export type RenderToString = (element: JSX.Element) => 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.
I left unknown as you should be able to use this argument with renderToStream or something as well, because we don't use the 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.
renderToReadableStream()
will probably create issues for example. But as long as its typed as a synchronous method it should be fine indeed.
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.
I was more thinking if there's in theory another method that traverses the tree, but doesn't render, it is also sufficient for this method
packages/react-instantsearch-hooks-server/src/getServerState.tsx
Outdated
Show resolved
Hide resolved
packages/react-instantsearch-hooks-server/src/__tests__/modules-none.test.tsx
Outdated
Show resolved
Hide resolved
packages/react-instantsearch-hooks-server/src/__tests__/modules-none.test.tsx
Outdated
Show resolved
Hide resolved
packages/react-instantsearch-hooks-server/src/__tests__/modules-none.test.tsx
Outdated
Show resolved
Hide resolved
…s-none.test.tsx Co-authored-by: François Chalifour <francoischalifour@users.noreply.github.com>
Followup on #3658, to be merged close to the release
follow up on algolia/react-instantsearch#3658 to be merged once this is released
Followup on #3658, to be merged close to the release EDIT: actually this can be merged before the release already as previously the unused argument wasn't used
follow up on algolia/react-instantsearch#3658 to be merged once this is released
Summary
There are some cases where the combination of trying to make sure renderToString doesn't end up in a browser bundle, being runnable on esm + cjs, react 17 and 18, .js extension etc. blows up.
One of those is pnpm/vercel removing "unused" packages.
Result
This PR introduces a new argument
renderToString
togetServerState
so you can inject the dependency yourself, meaning the import is within your own code and won't be purged.fixes #3633
closes #3618
see vercel/next.js#40067
FX-1869