search

samvera-labs / ramp

Interactive, IIIF powered audio/video media player React components library. Styleguidist Docs: https://samvera-labs.github.io/ramp/
https://ramp.avalonmediasystem.org/
30 stars 5 forks source link

React Upgrade to Latest - 18.3/19 RC #618

Closed Dananji closed 1 month ago

Dananji commented 2 months ago

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

joncameron commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.

Dananji commented 2 months ago

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: https://github.com/facebook/react/issues/29898#issuecomment-2308849302

Dananji commented 2 months ago

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.

Dananji commented 1 month ago

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.