bunqCommunity / bunqDesktop

The unofficial, free and open source desktop application for the bunq API
https://bunqdesk.top
MIT License
272 stars 53 forks source link

(WIP) refactor: Redux toolkit + TypeScript #551

Open firstred opened 4 years ago

firstred commented 4 years ago

This original plan was a simple refactor to make state management as a whole more scalable, but eventually turned into a much bigger refactor. Therefore, I have pushed a WIP to check if this PR has any chance of getting merged.

bunqDesktop is, without a doubt, a brilliant piece of engineering, but with like many projects of this size, scalability is becoming an issue. I have tried to add extra reducers to Redux but found it difficult not to knock over other functionality. "Luckily," I have seen this problem numerous times before in my own projects. Some helpful tips and toolkits from the creators of Redux and React have helped me solve the problem.

With this pull request, I am trying to add the best parts to the project.

Here is what has helped me tremendously scale React and Redux:

Let me know what you think! It's probably a bit overwhelming, but I expect that it's a great way to increase maintainability and improve performance.

Crecket commented 4 years ago

Everything so far seems fine with me as I read through it quickly but I'll check things more thoroughly tomorrow 👍

firstred commented 4 years ago

Redux Toolkit is complaining about non-serializable objects being added to the state. Whereas with Redux, you can usually get away with doing that, you are not supposed to do that and Redux Toolkit (properly) crashes once you dispatch non-serializable actions. To avoid future problems, I have made several objects serializable.

I have:

firstred commented 4 years ago

Another TypeScript improvement.

interface IProps { }

class RuleCollectionChecker extends React.Component<ReturnType<typeof mapStateToProps> & ReturnType<typeof mapDispatchToProps> & IProps> {
    // ...
}

const mapStateToProps = (state: ReduxState) => ({
    someProp: state.someProps,
});

const mapDispatchToProps = (dispatch: AppDispatch) => ({
    someDispatch: () => dispatch(someAction()),
});
firstred commented 4 years ago

I am currently trying to figure out the structure of the whole Redux state. You will notice that with the latest commit everything builds, but it is having a hard time listing payments, etc. That's because with Redux Toolkit I have to manually apply serialization before objects can be dispatched. It's currently using the fromPlainObject and toPlainObject methods of models for that, but I think toReduxObject and fromReduxObject will make it more obvious that we are serializing for Redux and not e.g. the API.

firstred commented 4 years ago

Although we could apply the same serialization as the API, I don't think that we should. I have been able to drastically reduce CPU and memory usage by applying a manual serialization strategy for the Redux store. It looks like, together with async and batch dispatching, these are going to be the biggest performance improvements we can achieve in the short term, with little effort.

firstred commented 4 years ago

Ahh and avoiding no serialization at all, obviously, is going to be a big improvement, too :smile_cat:

This seems to be the biggest performance hog: Screenshot from 2020-01-21 20-01-03