NotificationsYou must be signed in to change notification settings
Fork928
Star10.1k

Upgrade frontend to React 18#3353

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

ammario merged 20 commits intomainfromreact-18

Aug 22, 2022

Merged

Upgrade frontend to React 18#3353

ammario merged 20 commits intomainfromreact-18

Aug 22, 2022

Conversation

Copy link

Member

ammario commentedAug 2, 2022

Closes#961

ammario requested a review froma team as acode owner

August 2, 2022 07:00

ammario requested a review frompresleyp

August 2, 2022 07:23

ammario force-pushed thereact-18 branch from8dd15bc toa18c641Compare

August 2, 2022 07:35

Copy link

Member

Kira-Pilot commentedAug 2, 2022

@ammario thanks for doing this! We wanted to discuss some of the React 18 changes in FE Variety before merging up - there are some rendering differences between 17 and 18. Do you mind if we hold off until then? We can mark it as a draft for now.

Kira-Pilot reviewed

Aug 2, 2022

View reviewed changes

site/package.jsonShow resolvedHide resolved

Kira-Pilot reviewed

Aug 2, 2022

View reviewed changes

site/src/__mocks__/react-markdown.tsx OutdatedShow resolvedHide resolved

Copy link

MemberAuthor

ammario commentedAug 2, 2022

@ammario thanks for doing this! We wanted to discuss some of the React 18 changes in FE Variety before merging up - there are some rendering differences between 17 and 18. Do you mind if we hold off until then? We can mark it as a draft for now.

How about I fix these rendering concerns right now?

Copy link

Member

Kira-Pilot commentedAug 2, 2022

How about I fix these rendering concerns right now?

Well, it's not so much fixing them (although there might be some of that) so much as it is understanding them. Here is theReact article discussing the changes we are most curious about. Some questions I have (although I haven't finished reading this through) are:

how will this work when running tests locally, especially considering we often make use of Jest'stoBeCalledTimes?
Are there workarounds? Should we be limiting use ofuseEffect (probably)? There's a whole issue threadhere.

None of that necessarily needs to be addressed in this PR - I just think usually we give folks a chance to read through the change logs before upgrading.
Would you like to join our next FE variety? We all committed to discuss the reading then. It would be nice to hear your perspective considering you did the upgrade already!

Copy link

Contributor

presleyp commentedAug 3, 2022

Yeah we had decided to discuss this in FE V so I'd like to go ahead and do that. It's best to take tickets that have been groomed - this one was still in Enqueue because we were waiting on the discussion.

Copy link

MemberAuthor

ammario commentedAug 3, 2022

Ok. I'm happy to attend the next one. Please add me to the invite.

Copy link

MemberAuthor

ammario commentedAug 3, 2022

I intend on handling this end to end so it shouldn't add any work to your plates.

Copy link

Member

Kira-Pilot commentedAug 5, 2022

@presleyp can add you to the invite!

ammario added8 commits

August 15, 2022 15:37

Upgrade to React 18

d889b18

Update react-dom

7c5c2a1

Replace react-helmet with react-helmet-async

54d5cce

Chase down errors

6676cb3

Simplify FC defs

8a0a73f

Run prettier

4928cf7

Pin all new deps

8358053

Import PropsWithChildren in a few components

25d747d

ammario force-pushed thereact-18 branch from0eb7d12 to25d747dCompare

August 15, 2022 15:51

Copy link

socket-securitybot commentedAug 15, 2022•
edited
Loading

Socket Security Report

Dependency issues detected. If you merge this pull request, you will not be alerted to the instances of these issues again.

📜 New install scripts detected

A dependency change in this PR is introducing new install scripts to your install step.

Package	Script field	Location
core-js@3.24.1 (upgraded)	`postinstall`	`site/package.json` via@storybook/addon-actions@6.5.9
core-js-pure@3.24.1 (upgraded)	`postinstall`	`site/package.json` via@pmmmwh/react-refresh-webpack-plugin@0.5.7

Socket.dev scan summary

Issue	Status
Did you mean?	✅ no new possible package typos
Install scripts	⚠️ 2 new install scripts detected
Telemetry	✅ no new telemetry
Troll package	✅ no new troll packages
Malware	✅ no new malware
Native code	✅ no new native modules

Powered bysocket.dev

ammarioand others added6 commits

August 15, 2022 17:32

Fix remaining errors and most tests

5495f83

Resolve more tests

78448c4

Things are borked

fafff02

removing act; cleaning up tests

dab2f24

fixed UsersPage tests

df998c2

fixed CreateWorkspacePage tests

876e740

Kira-Pilot reviewed

Aug 18, 2022

View reviewed changes

site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx

		<ErrorSummary
		error={props.createWorkspaceErrors[CreateWorkspaceErrors.GET_TEMPLATES_ERROR]}
		/>
		) : (
		<></>

Copy link

Member

Kira-PilotAug 18, 2022

Choose a reason for hiding this comment

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

Instead of rendering a fragment here if the condition isn't met, we should be able to do this:

       {!!props.createWorkspaceErrors[CreateWorkspaceErrors.GET_TEMPLATES_ERROR] && (          <ErrorSummary            error={props.createWorkspaceErrors[CreateWorkspaceErrors.GET_TEMPLATES_ERROR]}          />        )}

The error message you're encountering is TS's confusing way of telling you that it's not evaluating the condition as a boolean, so it needs you to explicitly cast it as such.