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

React 18 #7617

Merged
merged 11 commits into from
Aug 30, 2022
Merged

React 18 #7617

merged 11 commits into from
Aug 30, 2022

Conversation

pettinarip
Copy link
Member

@pettinarip pettinarip commented Aug 29, 2022

Upgrade react to v18.

Description

Upgraded:

  • react & react-dom to v18
  • gatsby to v4.21.1
  • gatsby-plugin-image to v2.21.0
  • gatsby-plugin-sharp & gatsby-transformer-sharp to v4.21.1
  • @chakra-ui/react to v2
  • embla-carousel-react to v7
  • gatsby-plugin-mdx to v3
    • v3 is react 18 compatible
    • Note: there is a v4 already that supports mdx v2 but there are so many breaking changes that I have decided not to include it here
  • gatsby-plugin-sitemap & gatsby-remark-autolink-headers we are already using the latest major: v5
    • Bumped to new minor version v5.21.0
  • focus-trap-react bumped to v10
    • There are breaking changes but none of them impact our implementation. Nothing has changed from our side

Not upgraded:

  • gatsby-theme-i18n has not been upgraded yet
  • react-intl keeping current v3
  • react-select keep using v4, no breaking changes with React v18 (only the peer dep warning)
  • @types/react-instantsearch-dom has not been upgraded yet, removing the old one to avoid TS errors
  • react-helmet
    • one of its dependency react-side-effect has not been updated yet. They are still using an old version that only supports react 17.

Other fixes:

  • FeedbackWidget - mismatch in hydration step
  • Emoji - the underlying lib react-emoji-render has a mismatch issue in hydration step
    • Couldn’t find a solution yet, will raise an issue on the repo but it is not maintained anymore I think.
    • Check for alternatives. Self hosted options.
    • Note: perhaps we could use the flag suppressHydrationWarning in the meanwhile

@vercel
Copy link

vercel bot commented Aug 29, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
ethereum-org-website ✅ Ready (Inspect) Visit Preview Aug 29, 2022 at 5:53PM (UTC)

@github-actions github-actions bot added dependencies 📦 Changes related to project dependencies review needed 👀 labels Aug 29, 2022
@gatsby-cloud
Copy link

gatsby-cloud bot commented Aug 29, 2022

Gatsby Cloud Build Report

ethereum-org-website-dev

🎉 Your build was successful! See the Deploy preview here.

Build Details

View the build logs here.

🕐 Build time: 29m

@@ -92,7 +92,6 @@
"minimist": "^1.2.6",
"prettier": "^2.2.1",
"pretty-quick": "^3.1.0",
"react-test-renderer": "^17.0.1",
Copy link
Member Author

Choose a reason for hiding this comment

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

not used anymore

@@ -90,7 +91,7 @@ const CalloutBanner: React.FC<IProps> = ({
<Image
image={image}
alt={alt}
maxImageWidth={maxImageWidth}
maximagewidth={maxImageWidth}
Copy link
Member Author

Choose a reason for hiding this comment

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

I was getting a warning about this prop name and it suggested me to lowercase it.

const [isOpen, setIsOpen] = useState<boolean>(false)
const [feedbackSubmitted, setFeedbackSubmitted] = useState<boolean>(false)
const [isHelpful, setIsHelpful] = useState<boolean | null>(null)

const location = typeof window !== "undefined" ? window.location.href : ""
Copy link
Member Author

@pettinarip pettinarip Aug 29, 2022

Choose a reason for hiding this comment

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

This line was causing hydration problems. On server side this was generating a different output than in client side.

That is why I moved it into the useEffect.

@@ -3,7 +3,7 @@ import PropTypes from "prop-types"
import { Helmet } from "react-helmet"
import { useStaticQuery, graphql } from "gatsby"
import { useIntl } from "react-intl"
import { Location } from "@reach/router"
import { useLocation } from "@reach/router"
Copy link
Member Author

Choose a reason for hiding this comment

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

Was getting an error on the Location component that was related with React 18.

The types returned by 'render()' are incompatible between these types.

So, I have changed it to use the recommended approach, by using hooks.

@@ -83,6 +83,7 @@ const StyledEmoji = styled(Emoji)`
`

export interface IProps {
children?: React.ReactNode
Copy link
Member Author

Choose a reason for hiding this comment

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

children was removed from React.FC. DefinitelyTyped/DefinitelyTyped#56210

I'm starting to think about not using React.FC anymore since it doesn't bring any major benefit tbh. (CRA already removed it form their templates facebook/create-react-app#8177)

Copy link
Member

Choose a reason for hiding this comment

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

This is interesting, thanks for dropping that link

@nloureiro
Copy link
Contributor

On my side, the bug with theme switching it's fixed with this PR

Copy link
Member

@wackerow wackerow left a comment

Choose a reason for hiding this comment

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

This is looking good to me! Nice job @pettinarip!

Building nicely, code looks good... was hunting and pecking around the site to try and spot any issues and not seeing anything broken.

@@ -83,6 +83,7 @@ const StyledEmoji = styled(Emoji)`
`

export interface IProps {
children?: React.ReactNode
Copy link
Member

Choose a reason for hiding this comment

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

This is interesting, thanks for dropping that link

const intl = useIntl()

const desc = description || translateMessageId("site-description", intl)

const siteTitle = translateMessageId("site-title", intl)

/* Set canonical URL w/ language path to avoid duplicate content */
Copy link
Member

Choose a reason for hiding this comment

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

Nice, thanks for pulling all this logic out of the return statement

@pettinarip
Copy link
Member Author

This is looking good to me! Nice job @pettinarip!

Building nicely, code looks good... was hunting and pecking around the site to try and spot any issues and not seeing anything broken.

Nice, thanks for the review @wackerow. I'll play around a bit more to see if everything is ok and then will merge it.

@pettinarip pettinarip merged commit fd1bb29 into dev Aug 30, 2022
@pettinarip pettinarip deleted the react-18 branch August 30, 2022 21:30
@pettinarip pettinarip mentioned this pull request Aug 31, 2022
@corwintines corwintines mentioned this pull request Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies 📦 Changes related to project dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants