dhis2 / aggregate-data-entry-app

Data entry app for DHIS 2
https://dhis2-data-entry.netlify.app
BSD 3-Clause "New" or "Revised" License
3 stars 3 forks source link

Suggestions for technical improvements #133

Closed HendrikThePendric closed 2 years ago

HendrikThePendric commented 2 years ago

After the first release of this app, we need to probably take some time to refactor some parts of this app. This GitHub issue can be a place where we collect things we would like to see improved. We can comment here about things that need to be optimised, parts of the code that are hard to work with, or need better test coverage. If you already have a solution, or a global direction of how to address the problem in mind, please include that in the comment.

HendrikThePendric commented 2 years ago

We should probably add a state library like redux so we can have all the application state in one place and are able to subscribe to "slices" of this state in components.

HendrikThePendric commented 2 years ago

We should probably introduce a DataEntryFormInstanceProvider to which we pass a the current react-final-form Form instance. This way the entire application can easily interact with the form and its individual fields using the FormApi. I think that some of the providers we currently have could probably be removed if we had this, and we can also reduce the amount of re-renders this way (I am mainly thinking about using form.registerField and form.subscribe to do this).

HendrikThePendric commented 2 years ago

We could switch to final-form-calculate for indicators and totals

HendrikThePendric commented 2 years ago

We should look into ways of make the useDataValues() hook return a data object that changes less frequently. Potentially we could use this structural sharing approach to achieve that goal.

Disclaimer: it was @Birkbjo who suggested this to me.

HendrikThePendric commented 2 years ago

From the perspective of the data entry fields (i.e. the inputs), it would make a lot of sense if the entire form-field state was provided by react-final-form. We could take the following approach to achieve this:

This way, we can easily update individual fields from the useQuey hooks and we can keep all field data in a single place. I think this would help a lot.

Birkbjo commented 2 years ago

From the perspective of the data entry fields (i.e. the inputs), it would make a lot of sense if the entire form-field state was provided by react-final-form. We could take the following approach to achieve this: Allow other parts of the app to interact with the form by using the aforementioned DataEntryFormInstanceProvider provider Use a RFF Fields data prop to store information needed by the field, but not strictly speaking part of a form field. For example sync status etc. Use add mutators to the RFF Form which allow updating these meta fields (aka the data prop). This way, we can easily update individual fields from the useQuey hooks and we can keep all field data in a single place. I think this would help a lot.

If we get structural sharing to work properly I don't think this is necessary. Then react-query would be the only source of truth (except for form-state). And we could use useQuery-targetting specific fields without worrying about re-rendering.

I haven't fully investigated how this works, or if it does. I just think that if it works the way I hope - we don't have to deal with these points at all. Each field could just useQuery and select the part of the cache it wants.

However, one worry could be the amount of subscriptions. If that causes trouble, I agree we could implement some of your points - but the introduction of a DataEntryFormInstanceProvider seems a bit counter-intuitive to me, we should try to have one source of truth, instead of having to sync these manually.

HendrikThePendric commented 2 years ago

Moving this to a discussion here https://github.com/dhis2/data-entry-app/discussions/134