classtranscribe / FrontEnd

The React + Redux Frontend for ClassTranscribe
https://classtranscribe.illinois.edu
Other
26 stars 28 forks source link

Remove legacy dvajs ; unblock react update #768

Open angrave opened 9 months ago

angrave commented 9 months ago

dvajs is legacy, unmaintained, poorly documented and preventing us from moving to a more recent react version. It is likely that modern react components mean we don't need it.

We are using it for routing, history, and redux connect. However there are warnings in the browser console that are likely from dva -

Warning: Please use `require("history").createHashHistory` instead of `require("history/createHashHistory")`. Support for the latter will be removed in the next major release.

Warning: componentWillMount has been renamed, and is not recommended for use. See https://fb.me/react-unsafe-component-lifecycles for details.

* Move code with side effects to componentDidMount, and set initial state in the constructor.
* Rename componentWillMount to UNSAFE_componentWillMount to suppress this warning in non-strict mode. In React 17.x, only the UNSAFE_ name will work. To rename all deprecated lifecycles to their new names, you can run `npx react-codemod rename-unsafe-lifecycles` in your project source folder.

Please update the following components: Route, Router, Switch
angrave commented 9 months ago

A quick test of changing a component (not a screeen) from dva to react-redux leads to runtime error when the component is displayed. e.g. CTPopup.jsx

import { connect } from 'react-redux'; // 'dva' In the console-

Uncaught Error: Could not find "store" in the context of "Connect(CTPopup)". Either wrap the root component in a <Provider>, or pass a custom React context provider to <Provider> and the corresponding React context consumer to Connect(CTPopup) in connect options. This is surprising because (based on some quick searches) dvajs appears to just export redux's connect. Perhaps we're skipped to a newer version of redux (or react-redux) than what dvajs specifies internally (7.1.5)? Or some automatic glue that occurs when js imports dva??

https://github.com/dvajs/dva/blob/968da6a92a5891ab54cd82397b8ca22fa3498f67/packages/dva/src/index.js#L7

harsh183 commented 4 months ago

Looks like dvaJS just added support for React 18 and I don't think it's blocking a node upgrade either, so we can actually hit those first, upgrade our other dependencies and then tackle dvaJS.

I do think it's still worth removing since the React world has largely moved on from the conventions of then, and newer libraries have solved the issues dva solved. A lot of the documentation isn't in English and all the other reasons of removing it still hold. Removing dvaJS has a few different parts. Their official documentation says:

redux, redux-saga and react-router

Removing DvaJS will be in a few parts of just using the underlying libraries by themselves instead of dva itself.

For each pattern we can maybe write tests for 2-3 files for existing behavior, then replace the dvajs calls with the underlying library calls to make sure it still works.

harsh183 commented 3 months ago

More motivation to remove dva: https://github.com/classtranscribe/FrontEnd/pull/648 😩

harsh183 commented 3 months ago

Tracking react upgrade here: https://github.com/classtranscribe/FrontEnd/issues/818, maybe after this, we can tackle removing dvajs

harsh183 commented 3 months ago

https://umijs.org/ - looks like the Chinese big tech community has largely moved onto umi which takes the dva react concepts further and creates a tighter integration with fewer APIs required. So looks DVA is largely a thing of the past now from what I can tell