-
Notifications
You must be signed in to change notification settings - Fork 386
feat(hooks-server): support import from React 18 #3464
Conversation
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 4d30a0c:
|
✅ Deploy Preview for react-instantsearch ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
packages/react-instantsearch-hooks-server/src/getServerState.tsx
Outdated
Show resolved
Hide resolved
packages/react-instantsearch-hooks-server/src/getServerState.tsx
Outdated
Show resolved
Hide resolved
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.
Clean!
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-with-extension.test.tsx
Outdated
Show resolved
Hide resolved
packages/react-instantsearch-hooks-server/src/__tests__/modules-with-extension.test.tsx
Outdated
Show resolved
Hide resolved
packages/react-instantsearch-hooks-server/src/__tests__/modules-with-extension.test.tsx
Outdated
Show resolved
Hide resolved
packages/react-instantsearch-hooks-server/src/__tests__/modules-without-extension.test.tsx
Outdated
Show resolved
Hide resolved
packages/react-instantsearch-hooks-server/src/__tests__/modules-without-extension.test.tsx
Outdated
Show resolved
Hide resolved
packages/react-instantsearch-hooks-server/src/__tests__/modules-without-extension.test.tsx
Outdated
Show resolved
Hide resolved
Not sure why the CI is failing as if one of the modules was present, it passes locally. |
Co-authored-by: Sarah Dayan <5370675+sarahdayan@users.noreply.github.com>
Not sure what happens, I hope the test isn't flaky (it isn't on my machine), but in the second run after those wording changes were made it did pass @sarahdayan |
Okay that's odd, but could be a cache issue on Circle maybe? I guess we can move forward and we'll see soon enough if the test is flaky or not. |
I've just confirmed that this works as expected in a fresh next app with react 18: https://codesandbox.io/s/nqhmo4 (before I tested within the monorepo) |
Summary
Import both
react-dom/server.js
(react 16 and 17) andreact-dom/server
(react 18, since they added "exports" in package.json) dynamically to make sure server-side rendering works in all situationsResult
fixes #3453
FX-1405
I don't see how this can be tested immediately, but we can add a react 18 next.js example later along the existing react 17 one, which would fail before this PR
We were slightly worried this may hurt TTFB, but in reality that doesn't seem to be the case, as evidenced by a quick benchmark (using https://github.com/mildsunrise/curl-benchmark) done: