refactor: optimize re-renders

[Birkbjo]

Refactors the header in to a separate component. This PR is meant to make the CategoryComboTable-component a bit easier to reason about, as well as solve some re-renders.

• useActiveCell caused entire table to re-render
  • This is now only re-renders header-component and DataElementCell (header of the data-element)
• CurrentItemContext caused all cells to re-render because they subscribed to this context
  • Splitting this to a separate "setter"-context, solves this, as the cells only need to set the item

[dhis2-bot]

🚀 Deployed on https://pr-87--dhis2-data-entry.netlify.app

[HendrikThePendric]

Review

Great work @Birkbjo 🦾

I ran the app locally with my CPU throttled 4x. It is already much better than it was, and we still can make more performance optimisations in other areas (i.e. not resetting the initial values). So I'm feeling quite confident we can get this app optimised without having to change too much.

I left a few minor comments. Looking forward to finding out what you think about them.

---

Side note

When I tried the Reproductive Health form (which is quite a large form, so I tend to use it for performance checks), I did notice something strange though....

There are some empty cells at the end of the table, which don't seem to be needed....

The old app doesn't have this issue so I guess we have a bug in our category-combo-table. Shall we try to fix this in the current PR, or shall I open a JIRA ticket for it?

[HendrikThePendric]

I'm thinking that it would probably benefit the app's architecture and performance if we were to push this form state subscription down to the level of the actual header cell.

[Birkbjo]

I don't really think it's necessary to push it further. You could in theory have a CategoryTableHeaderColumns and move it there, and basically move the rendered component from columns.map.... But that would only save re-rendering of the static components "Category name" and "PaddingCell". Again these are static, with no dynamic props and no DOM-updated are needed.

[HendrikThePendric]

I was mainly concerned about the nested iterations needed to compute the columns, and that fact that they would need to run whenever a the active cell changes. I suggested solving this by pushing the subscription to the active cell down, but I noticed you are now memoizing these columns in the useCategoryColumns. I think that's an equally valid solution.

[HendrikThePendric]

I was just wondering why we also need to isThisTableActive const. Is it possible that a form contains several tables with the same dataElements?

Look at this very naively, I would expect that every cell will be unique so we don't need this variable per se. If you have added that isThisTableActive variable to make the code more performant, then you should add it as an early return in the isHeaderActive function.

Having said all of that, if you agree with my earlier suggestion, about moving the active/inactive logic down to the actual header cell, then this code is going to change anyway.

[Birkbjo]

I did some refactoring to not need to pass all dataElements to this component. So I pass a function checkTableActive instead. But yes, it's needed because section-forms render multiple tables, and we only want to highlight the active table.

Is it possible that a form contains several tables with the same dataElements?

No, and that's why this fact is used to check if the current table is "active" or not.

then you should add it as an early return in the isHeaderActive function.

I don't really understand what you mean here? This is only a part of the check isHeaderActive. It's not enough to know if the current table is active. You need to also check if the current active cell is in the correct columns AND row as well. This code is for highlighting of the headers.

[HendrikThePendric]

I did some refactoring to not need to pass all dataElements to this component. So I pass a function checkTableActive instead. But yes, it's needed because section-forms render multiple tables, and we only want to highlight the active table.

Is it possible that a form contains several tables with the same dataElements?

No, and that's why this fact is used to check if the current table is "active" or not.

Partly from your explanation above, and by looking at the code a bit more closely, I get this now 👍 .

---

then you should add it as an early return in the isHeaderActive function.

I don't really understand what you mean here?

I was mainly just figuring out what was happening, and my suggestion about returning early was basically just this (see the comment):

    const isHeaderActive = (headerIdx, headerColSpan) => {
        if (!checkTableActive(activeDeId)) {
            return false
        }
        // by returning early you can avoid this iteration
        const activeCellColIdx = categoryOptionCombos.findIndex(
            (coc) => activeCocId === coc.id
        )
        const idxDiff = activeCellColIdx - headerIdx * headerColSpan
        return (idxDiff < headerColSpan && idxDiff >= 0)
    }

But this is very much a micro-optimisation, so I'm very happy to skip this.

[Birkbjo]

There are some empty cells at the end of the table, which don't seem to be needed....

The old app doesn't have this issue so I guess we have a bug in our category-combo-table. Shall we try to fix this in the current PR, or shall I open a JIRA ticket for it?

This is a backend issue. Seems like someone has changed the categoryCombo, and as you probably know there's plenty of issues when doing this after it's created.

This is also logged in the console:

Computed combination of categoryOptions for catCombo(m2jTvAj5kkm) is different from server.
            Please regenerate categoryOptionCombos.
            Computed: 12
            Server: 24) 

[HendrikThePendric]

Thanks for clarifying some things all good now 👍

Reg. this:

The old app doesn't have this issue so I guess we have a bug in our category-combo-table. Shall we try to fix this in the current PR, or shall I open a JIRA ticket for it?

Let's just defer this, since it's unrelated to your current work. I created TECH-1255 for this.

[HendrikThePendric]

I was mainly concerned about the nested iterations needed to compute the columns, and that fact that they would need to run whenever a the active cell changes. I suggested solving this by pushing the subscription to the active cell down, but I noticed you are now memoizing these columns in the useCategoryColumns. I think that's an equally valid solution.

[HendrikThePendric]

I did some refactoring to not need to pass all dataElements to this component. So I pass a function checkTableActive instead. But yes, it's needed because section-forms render multiple tables, and we only want to highlight the active table.

Is it possible that a form contains several tables with the same dataElements?

No, and that's why this fact is used to check if the current table is "active" or not.

Partly from your explanation above, and by looking at the code a bit more closely, I get this now 👍 .

---

then you should add it as an early return in the isHeaderActive function.

I don't really understand what you mean here?

I was mainly just figuring out what was happening, and my suggestion about returning early was basically just this (see the comment):

    const isHeaderActive = (headerIdx, headerColSpan) => {
        if (!checkTableActive(activeDeId)) {
            return false
        }
        // by returning early you can avoid this iteration
        const activeCellColIdx = categoryOptionCombos.findIndex(
            (coc) => activeCocId === coc.id
        )
        const idxDiff = activeCellColIdx - headerIdx * headerColSpan
        return (idxDiff < headerColSpan && idxDiff >= 0)
    }

But this is very much a micro-optimisation, so I'm very happy to skip this.

[HendrikThePendric]

I hadn't seen your last comment yet when I approved, and was creating a new JIRA issue for the empty columns.....

This is a backend issue. Seems like someone has changed the categoryCombo, and as you probably know there's plenty of issues when doing this after it's created.

I'll make sure to add this comment to the JIRA ticket, but will actually keep the JIRA ticket open. Even though it may be a backend issue, the old app deals with these types of misconfigurations gracefully and we don't. So I'm guessing we will need to fix it somewhere.

[Birkbjo]

I was mainly just figuring out what was happening, and my suggestion about returning early was basically just this (see the comment):

const isHeaderActive = (headerIdx, headerColSpan) => {
    if (!checkTableActive(activeDeId)) {
        return false
    }
    // by returning early you can avoid this iteration
    const activeCellColIdx = categoryOptionCombos.findIndex(
        (coc) => activeCocId === coc.id
    )
    const idxDiff = activeCellColIdx - headerIdx * headerColSpan
    return (idxDiff < headerColSpan && idxDiff >= 0)
}

But this is very much a micro-optimisation, so I'm very happy to skip this.

Ah, I understand now! Yeah we could indeed do that, but yeah probably very much a micro-optimisation. categoryOptionCombos.length would probably never be a big number. They are basically the number of columns in the table.
But I appreciate the thought, since this is a optimization PR afterall!

[Mohammer5]

Code generally looks good! Despite approving (as it seems everything works as expected), I left one change request regarding abbreviations.

I have one question for my own understanding: Why is using two providers (one "getter provider" and one "setter provider") functionally different from using a single provider? As I understanding it, it does seem to make a different, I just don't understand why yet. Could you explain that? Maybe we can put the final explanation that we end up with as a comment in the current-item-provider.js (where the two providers are being nested)

[Mohammer5]

Actually, in regards to the provider thing, Dan Abramov said something about this: facebook/react#15156 (comment)

Option 2 seems to be similar to something I've suggested before: The only component inside the form that needs the context is <EntryFieldInput />. Instead of using two providers, inside the entry field input module, have a wrapper component that extracts the setter while the actual entry field input expects the setter as prop and is wrapped with memo. That seems more canonical to me.

I guess as the provider stuff already works, I don't see a need to change that now though! Just wanted to mention that there seems to be a more "official" solution as it's coming from Dan, might be worth considering that solution if we end up in this situation again

[Birkbjo]

Actually, in regards to the provider thing, Dan Abramov said something about this: facebook/react#15156 (comment)

Option 2 seems to be similar to something I've suggested before: The only component inside the form that needs the context is <EntryFieldInput />. Instead of using two providers, inside the entry field input module, have a wrapper component that extracts the setter while the actual entry field input expects the setter as prop and is wrapped with memo. That seems more canonical to me.

I guess as the provider stuff already works, I don't see a need to change that now though! Just wanted to mention that there seems to be a more "official" solution as it's coming from Dan, might be worth considering that solution if we end up in this situation again

Thanks for the link, that's helpful! But our solution is his "option 1" and preferred solution, so I don't really see how "option 2" is "more official"? He explicitly marks our current solution as the preferred solution? We are splitting our context - the value that is changing often, and the setter-function that never changes.

The other solutions will still cause re-renders. Yes, I guess they would be cheap - but they are still re-renders. So I have to say I prefer the current solution - even for the future. Of course, this is only needed if a value is subscribed to in a lot of components - this kind of optimisation shouldn't be that common.

We are rendering the extra context-provider in the same Provider-component, so having an extra context is not really something other components really need to worry about. We just expose a separate hook for the setter. So I would also argue this solution is cleaner than splitting the consumer component in a wrapper just for the purpose of subscribing there.

[Mohammer5]

But our solution is his "option 1" and preferred solution

You're right! I thought about this differently, I read this like "Split providers if they handle different things on a conceptual level", but of course, you're right, in our case the value and setter are not used together, so this makes perfect sense! Never mind me, then!