fix: remove FinalForm dependency in main form [DHIS2-18373] #425
Conversation
|
🚀 Deployed on https://pr-425--dhis2-data-entry.netlify.app |
| const [lastSyncedValue, setLastSyncedValue] = useState(() => | ||
| convertBooleanValue(initialValue) | ||
| ) | ||
| const [syncTouched, setSyncTouched] = useState(false) |
There was a problem hiding this comment.
In "genric-input" we just do setValueSynced(value === newValue). What is the reason that this and other fields need the effect and syncTouched? Could we not calculate this, or set the synced value in the onSuccess-callback? Or should the generic-input also use this way of doing it?
There was a problem hiding this comment.
I think it's more correct to do the side effect with useEffect. In the generic input, which is the first one I wrote, this setValueSynced(value === newValue) was working, but in the other components, it does not work, I suppose because the code is executing before the new value has actually been set.
There was a problem hiding this comment.
Alright, I just in general try to not use useEffect, and always try to look for other solutions. It adds additional re-renders and complexity. But maybe in this case it's warranted - because we're actually syncing states here.
But I do feel like the need for "syncTouched", "lastSyncedValue", and "setValueSynced" is somewhat complex.
Ideally this is just something we could calculate from mutation state, and potentially the query-cache. But it's hard to argue for such changes when this seems to work great.
| ) | ||
|
|
||
| const initialValuesInStore = |
There was a problem hiding this comment.
This is the logic to check that the values in the zustand store are for the given data form, so as to prevent rendering the input cells before the initial values are present
There was a problem hiding this comment.
I found a small (but scary) bug that I think is related to this, but also the service worker somehow.
If you edit a cell - blur and let it save - then refresh. The value will not be shown in the form.
You can also test this with another tab of the same form: if you update the form in one tab, then refresh in another - you won't get the updated data unless you refresh twice.
It seems like the forms initial values are populated with the "offline"-cached version of the values. So I think we need some additional checks for the loading in data-workspace?
We may need to check for "isFetchedAfterMount" on the initialDataValuesFetch query when checking for loading indicator - unless you're offline?
eg:
if (initialDataValuesFetch.isFetching || (!initialDataValuesFetch.isFetchedAfterMount && !initialDataValuesFetch.isPaused)) {
return (
<CenteredContent>
<CircularLoader />
</CenteredContent>
)
}
There was a problem hiding this comment.
😱 thanks for that!
I'm too be honest a little confused with the react-query nomenclature here. I would have thought that since we had refetchOnMount: false for this query that isFetchedAfterMount would remain false, but I guess it's 'isFetchedAfterMountand maybe I'm imaging a variableisRefetchedAfterMount`?
src/data-workspace/form-wrapper.js
Outdated
| useValueStore((state) => state.getInitialDataValues())?.formKey === | ||
| validFormKey | ||
|
|
||
| useLayoutEffect(() => { |
There was a problem hiding this comment.
I don't think it matters too much here, but I think normal useEffect should suffice.
| if (syncTouched) { | ||
| setValueSynced(value === lastSyncedValue) | ||
| } | ||
| }, [value, lastSyncedValue, syncTouched]) |
There was a problem hiding this comment.
🤦♂️ sorry for that. I'm impressed(?) with myself for managing to miss multiple lint warnings both locally and in github 😬
There was a problem hiding this comment.
Thanks a lot for fixing this! This is a lot of work and very impressive!
I've mainly a few comments about lint-issues and a bug that needs to be fixed.
I do feel like some of the sync logic could be hoisted to "DataEntryField". Eg. we could have a lastSyncedValue-state there. Then we could update that value, instead of a "synced"-boolean. And compare value (from the store) with lastSyncedValue. Right now we're comparing the state in the field, but it's the data in the dataValueStore that is actually "the source of truth", right? And in theory these could diverge?
But it might be that would complicate things - because I do see some logic to compare the values for eg. the file-field. So I won't block this PR because of this.
|
Thanks for the review (x2)! I've addressed the lint + bug issue 🙏
I think of this PR as a bug fix for the immediate issue of the forms being non-performant because of the react-final-form dependency (which we probably went with for convenience/familiarity/fact that it's called a data entry form even though it's not really a typical form), but it would be good to make a follow-up ticket (feature) to revisit some of the other logic to see if we can improve it further/leverage some of react-query's functionality for checking synced state. I do wonder though on the specific issue of whether it's more ideal to indicate sync status based on comparison with the input state or the dataValueStore. I would actually think that insofar as the sync state is being recorded to provide a visual cue in the cell, we'd probably want it to reflect what the user is seeing in the cell? Like if for some reason cell shows |
This addresses performance issues associated with Final Form by removing the FinalForm dependency from the main form. See the original ticket for more information about where performance problems occured: DHIS2-18373
Instead of using form/field state, the field inputs have been refactored
valueSynced: truewhen the cell's current state value matches the last synced valueTo simplify things, I've also used useHighlightedField in some places where we had looked for active field. This could be updated, but apart from the difference in border color of the cell, I wasn't sure if the differentiating between active/highlighted was so relevant. (It does slightly change the calculation of indicators/totals, but I think this is pretty minor and some of our active state management seemed a bit buggy on some input types)