Skip to content
This repository has been archived by the owner on Dec 30, 2022. It is now read-only.

feat(hooks-server): support import from React 18 #3464

Merged
merged 9 commits into from
May 12, 2022
Merged

Conversation

Haroenv
Copy link
Contributor

@Haroenv Haroenv commented May 11, 2022

Summary

Import both react-dom/server.js (react 16 and 17) and react-dom/server (react 18, since they added "exports" in package.json) dynamically to make sure server-side rendering works in all situations

Result

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:

Before the change:

              DNS      TCP        SSL  Request           Content
     Code  lookup  connect  handshake     sent    TTFB  download
     min:     9.0      0.0        0.0      0.0    52.0       0.0
     avg:    14.4      0.3        0.0      0.1    83.2       0.4
     med:    15.0      0.0        0.0      0.0    76.0       0.0
     max:    21.0      1.0        0.0      1.0   203.0       1.0
     dev:   23.2%   160.4%       0.0%   339.1%   35.3%    133.3%
           requests: 25    samples: 25    failures: 0

After the change:

              DNS      TCP        SSL  Request           Content
     Code  lookup  connect  handshake     sent    TTFB  download
     min:    10.0      0.0        0.0      0.0    44.0       0.0
     avg:    15.7      0.3        0.0      0.1    72.4       0.3
     med:    16.0      0.0        0.0      0.0    63.0       0.0
     max:    24.0      1.0        0.0      1.0   220.0       1.0
     dev:   23.0%   152.8%       0.0%   374.2%   51.6%    165.8%
           requests: 30    samples: 30    failures: 0 

@codesandbox-ci
Copy link

codesandbox-ci bot commented May 11, 2022

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:

Sandbox Source
react-instantsearch-app Configuration
hooks-example Configuration

@netlify
Copy link

netlify bot commented May 11, 2022

Deploy Preview for react-instantsearch ready!

Name Link
🔨 Latest commit 4d30a0c
🔍 Latest deploy log https://app.netlify.com/sites/react-instantsearch/deploys/627cf75b0a60e400089a0b39
😎 Deploy Preview https://deploy-preview-3464--react-instantsearch.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@Haroenv Haroenv marked this pull request as ready for review May 11, 2022 11:27
Copy link
Member

@sarahdayan sarahdayan left a comment

Choose a reason for hiding this comment

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

Clean!

@sarahdayan
Copy link
Member

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>
@Haroenv
Copy link
Contributor Author

Haroenv commented May 12, 2022

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

@sarahdayan
Copy link
Member

sarahdayan commented May 12, 2022

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.

@Haroenv Haroenv enabled auto-merge (squash) May 12, 2022 12:02
@Haroenv
Copy link
Contributor Author

Haroenv commented May 12, 2022

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)

@Haroenv Haroenv merged commit 0a13867 into master May 12, 2022
@Haroenv Haroenv deleted the feat/hooks-server-18 branch May 12, 2022 12:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

react-instantsearch-hooks-server doesn't work with react 18
4 participants