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 Upgrade to Latest - 18.3/19 RC #618

Closed
5 tasks done
Dananji opened this issue Aug 9, 2024 · 6 comments
Closed
5 tasks done

React Upgrade to Latest - 18.3/19 RC #618

Dananji opened this issue Aug 9, 2024 · 6 comments
Assignees
Labels
dependencies Pull requests that update a dependency file investigation Related research work

Comments

@Dananji
Copy link
Collaborator

Dananji commented Aug 9, 2024

Description

Currently Ramp doesn't support React 18.x, and fails to compile due to unsupported asynchronous implementation of useEffect hook in React. As React is upto 18.3 (latest) we need to add support for it.

Done Looks Like

  • Investigate effort to upgrade to React 18.3 and then again to React 19
  • Upgrade to React 18.3 and get code stable again
  • Change unsupported code: asynchronous React.useEffect()
  • All dependencies work with the upgrade
  • Attempt upgrade to React 19 RC (moved to a separate issue: React 19 Upgrade #638)
@Dananji Dananji added the dependencies Pull requests that update a dependency file label Aug 9, 2024
@joncameron
Copy link
Contributor

joncameron commented Aug 13, 2024

Breaking change from 17 to 18. Could be done after the refactoring work; could be done in parallel but doing this as soon as possible after state management and refactoring work would be best.

https://react.dev/blog/2024/04/25/react-19-upgrade-guide

@joncameron
Copy link
Contributor

We could do 18 upgrade now, and upgrade to 19 later—as long as it doesn't increase the effort required. We're probably very close to a React 19 release.

@elynema
Copy link

elynema commented Aug 28, 2024

If there will be some significant effort to go from React 18 to React 19 (including code changes to take advantage of 19 features we'd like to use), we can consider pulling the React 19 release candidate into Ramp and just waiting to cut a Ramp release until we can bring in the final stable version of React 19.

@elynema elynema closed this as completed Aug 28, 2024
@elynema elynema added the investigation Related research work label Aug 28, 2024
@Dananji Dananji reopened this Aug 28, 2024
@Dananji Dananji self-assigned this Aug 28, 2024
@Dananji
Copy link
Collaborator Author

Dananji commented Aug 28, 2024

From Chris in Slack regarding the issue blocking release of React 19:

FWIW It looks like the issue that was found in the React 19 RC is getting worked on right now: facebook/react#29898 (comment)

@Dananji
Copy link
Collaborator Author

Dananji commented Aug 28, 2024

Upgrading to React 19 RC from React 17:
According to the release notes for React 19 RC; it is recommended upgrading to React 18.3 before moving on to React 19 as React 18.3 has deprecation warnings for some things that are going to be deprecated in React 19. This could help identify breaking changes in advance to upgrading to 19.

Upgrading to React 18.3 from React 17:
There are some breaking changes including async React.useEffect() and deprecation of ReactDOM.render() in this upgrade.

Conclusion:
It seems there's a smaller difference between React 18.3 and React 19 or at least React 18.3 helps set up things required for a React 19 upgrade.
I think upgrading to React 18.3 first might help make the upgrading process easier.

So, we can do the 17 -> 18.3 -> 19 RC upgrade in that order for this ticket.
I think the effort for this is still at a 3, because it seems the extra step of upgrading from 18.3 to 19 RC is low effort.

@elynema elynema changed the title React Upgrade to Latest - 18.3 React Upgrade to Latest - 18.3/19 RC Sep 4, 2024
@Dananji
Copy link
Collaborator Author

Dananji commented Sep 6, 2024

Once the React 18.3.1 upgrade was done, I was seeing a bunch of warnings for the Video.js custom components. The use of ReactDOM createRoot and render in them were triggering this warning. To get rid of these warnings I converted almost all the components to extend on Video.js' exported components and to use provided functions to build the custom components. This way, we are not manually attaching the HTML into the DOM using ReactDOM functions. Since, VideoJSProgress component was too big, I kept it as it is.
I read more on the warning I was seeing for the bundled package, and I seemed the warning is due a bug in the build process which would be fixed in the future.

Once it was upgraded to React 19 RC, I was getting an error in the bundled package. This error seems to be popping up from the same place where I was seeing a warning with React 18.3.1 in the VideoJSProgress component. So, this warning was not something related to a bug in the build process as I had initially suspected.
Since, we will be upgrading to React 19 in the future, I started looking into fixing this.
It seems converting the way the VideoJSProgress component gets built into Video.js identical to the other components fixes this build error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file investigation Related research work
Projects
None yet
Development

No branches or pull requests

3 participants