Skip to content
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

[CORL-2451]: Update to React 18 #4056

Merged
merged 13 commits into from
Sep 27, 2022
Merged

Conversation

kabeaty
Copy link
Contributor

@kabeaty kabeaty commented Sep 21, 2022

What does this PR do?

These changes update Coral to use React 18. To achieve this, the following changes were required:

package updates

  • updating Prettier, which is why there are some formatting/linting changes throughout
  • updating @fluent/react, which included updates to Localized props. Vars are now all included in a vars prop and elements are now always included in an elems prop, instead of all separate props for each var/elem before.

A few other packages had to be updated, but these are the main ones that required significant changes along with their upgrades.

React updates

  • children has to be explicitly included in a component's props now
  • createRoot instead of ReactDOM.render

test updates

  • some tests had to be converted to React Testing Library
  • some tests needed additional acts/awaits

Note that unfortunately we cannot yet use createRoot for the stream. This is due to this issue in Virtuoso. So, the stream will use ReactDOM.render for now and be on React 17. Once this issue is resolved, we should be able to switch it over too.

What changes to the GraphQL/Database Schema does this PR introduce?

none

Does this PR introduce any new environment variables or feature flags?

no

If any indexes were added, were they added to INDEXES.md?

n/a

How do I test this PR?

You can test this by clicking around and seeing that everything still looks and works as expected in the stream/admin.

How do we deploy this PR?

@netlify
Copy link

netlify bot commented Sep 21, 2022

Deploy Preview for gallant-galileo-14878c canceled.

Name Link
🔨 Latest commit 6160f41
🔍 Latest deploy log https://app.netlify.com/sites/gallant-galileo-14878c/deploys/633382702eaea3000a4735ef

Comment on lines +3 to +35
const d3Pkgs = [
"d3",
"d3-array",
"d3-axis",
"d3-brush",
"d3-chord",
"d3-color",
"d3-contour",
"d3-delaunay",
"d3-dispatch",
"d3-drag",
"d3-dsv",
"d3-ease",
"d3-fetch",
"d3-force",
"d3-format",
"d3-geo",
"d3-hierarchy",
"d3-interpolate",
"d3-path",
"d3-polygon",
"d3-quadtree",
"d3-random",
"d3-scale",
"d3-scale-chromatic",
"d3-selection",
"d3-shape",
"d3-time",
"d3-time-format",
"d3-timer",
"d3-transition",
"d3-zoom",
];
Copy link
Contributor

Choose a reason for hiding this comment

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

this is cool, didn't know we were using d3 in places! 👍

Comment on lines +27 to 28
// eslint-disable-next-line
const supportedBrowsersRegExp = ${regexp};
Copy link
Contributor

Choose a reason for hiding this comment

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

Did it get mad about dumping a regexp into a formatted string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the updates to prettier/linting, without this, it will get stuck in a loop when trying to commit of needing to regenerate this, but if it's generated, then the linting complains. So, I disabled the linting just for this line to break it out of this loop.

const CoralContainer: FunctionComponent = ({ children }) => (
<CoralWindowContainer>{children}</CoralWindowContainer>
);
const CoralContainer: FunctionComponent<CoralContainerProps> = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

ah, the "you have children is not assumed by default in the types" begins... (lol)

@@ -81,7 +81,7 @@ const ResetPasswordForm: React.FunctionComponent<Props> = ({
</Localized>
<Localized
id="resetPassword-passwordDescription"
$minLength={8}
vars={{ minLength: 8 }}
Copy link
Contributor

Choose a reason for hiding this comment

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

well this is a pleasant improvement syntax wise, I didn't really like specifying by $<varname>... felt too much like PHP haha

Comment on lines +102 to +107
const [{ moderationQueueSort }] =
useLocal<ModerateCardContainerLocal>(graphql`
fragment ModerateCardContainerLocal on Local {
moderationQueueSort
}
`);
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like the new "recommended" typescript lint formatting has improved, I like the layout of this better. That or auto-formatting just wasn't run on this file in a while.

@@ -27,7 +27,10 @@ async function main() {
);

// eslint-disable-next-line no-restricted-globals
ReactDOM.render(<Index />, document.getElementById("app"));
const container = document.getElementById("app");
const root = createRoot(container!);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hah, this makes the 4th time they've changed the default "start doing React" part of React lol

@tessalt tessalt added this to the v7.3.2 milestone Sep 27, 2022
@tessalt tessalt added the 🚀 merge it! Pull requests that should be merged after status checks pass with a review label Sep 27, 2022
@kodiakhq kodiakhq bot merged commit 9e0c2b0 into develop Sep 27, 2022
@kodiakhq kodiakhq bot deleted the chore/CORL-2451-update-react branch September 27, 2022 23:25
@tessalt tessalt restored the chore/CORL-2451-update-react branch October 5, 2022 13:01
nick-funk added a commit that referenced this pull request Oct 5, 2022
This reverts commit 9e0c2b0.

# Conflicts:
#	src/core/client/stream/tabs/Comments/ReplyList/ReplyListContainer.tsx
#	src/core/client/stream/test/comments/stream/liveCommentReplies-rtl.spec.tsx
#	src/core/client/stream/test/comments/stream/showConversation.spec.tsx
#	src/core/client/stream/test/fixtures.ts
kabeaty added a commit that referenced this pull request Oct 6, 2022
kodiakhq bot pushed a commit that referenced this pull request Oct 6, 2022
* Revert "[CORL-2451]: Update to React 18 (#4056)"

This reverts commit 9e0c2b0.

* fix read more test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚀 merge it! Pull requests that should be merged after status checks pass with a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants