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

Remove legacy dvajs ; unblock react update #768

Open
angrave opened this issue Dec 20, 2023 · 5 comments
Open

Remove legacy dvajs ; unblock react update #768

angrave opened this issue Dec 20, 2023 · 5 comments
Assignees

Comments

@angrave
Copy link
Collaborator

angrave commented Dec 20, 2023

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 angrave changed the title Remove dva and update to more recent react Remove legacy dvajs ; unblock react update Dec 20, 2023
@angrave
Copy link
Collaborator Author

angrave commented Dec 20, 2023

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

@michaelxu12345 michaelxu12345 self-assigned this Jan 22, 2024
@harsh183 harsh183 self-assigned this Jun 2, 2024
@harsh183
Copy link
Member

harsh183 commented Jun 3, 2024

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.

  • React Router 'dva/router' 39 results
  • Redux Saga dva/saga (about 11 files)
    • effect: blocks with *functions as well as delay used once
  • Redux connect : 52 files
    • These are using the older connect pattern anyway, so we'd follow a guide like this when rewriting
  • Models: 10 ish files (wrapping the whole thing around)
    • These will get smaller and smaller as we remove the rest above, the model is basically just the entry point for the dvaJS.

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 harsh183 mentioned this issue Jun 4, 2024
@harsh183
Copy link
Member

harsh183 commented Jun 8, 2024

More motivation to remove dva: #648 😩

@harsh183
Copy link
Member

harsh183 commented Jun 8, 2024

Tracking react upgrade here: #818, maybe after this, we can tackle removing dvajs

@harsh183
Copy link
Member

harsh183 commented Jun 8, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants