WP-4924 Redraw only once when store triggers along with ancestor rerender

[greglittlefield-wf]

Ultimate Problem

Flux components were rerendered excessively when nested inside each other and a store trigger caused rerendering of those nested components.

See reduced test case here.

Solution

Track whether components were already redrawn during the current redraw batch.

All consumers will need to do is make sure to call super.componentDidUpdate in overrides, and they'll get this fix.

OverReact PR (FluxUiComponent) is available here: Workiva/over_react#103.

Testing

Verify tests pass.

Areas of Regression

Flux component rerendering

---

FYA @Workiva/web-platform-pp @Workiva/ui-platform-pp
FYI: @kylemcmorrow-wf @johnbland-wf @brentschuele-wf @brandonrodgers-wf @robbielamb-wf

[aviary2-wf]

Raven

Number of Findings: 0

[codecov-io]

Codecov Report

Merging #96 into master will increase coverage by 0.19%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #96      +/-   ##
==========================================
+ Coverage   96.77%   96.96%   +0.19%     
==========================================
  Files           5        5              
  Lines          62       66       +4     
==========================================
+ Hits           60       64       +4     
  Misses          2        2

Impacted Files	Coverage Δ
lib/src/mixins/batched_redraws.dart	100% <100%> (ø)	⬆️
lib/src/component_client.dart	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da81b7a...4c7f4ad. Read the comment docs.

[joelleibow-wf]

I can't wrap my head around why you have to iterate again. Can you help me understand that?

[greglittlefield-wf]

Sure thing. We want to unset this flag only after all of the batched redraws (and the redraws they cause by rerendering their descendants) have completed.

Otherwise, if we just unset the flag after a component redraw, it's possible that subsequent redraws in the batch will cause redrawing of that component again.

[maxwellpeterson-wf]

@jayudey-wf ready for you

[jayudey-wf]

would one of you guys want to verify that this update is functioning as you expect?

@kylemcmorrow-wf @johnbland-wf @brentschuele-wf @brandonrodgers-wf @robbielamb-wf

[kylemcmorrow-wf]

@jayudey-wf

I think it looks good.
I ended up pulling those changes into graph_app and I'm seeing some extra renders (after removing the manual shouldBatchRedraw switching), but I think that might be unrelated specifically to batch redraw (so we might have to keep some of the manual switching).

[greglittlefield-wf]

@kylemcmorrow-wf To clarify, did you just pull these changes in, or also the FluxUiComponent ones in the over_react PR Workiva/over_react#103?

[kylemcmorrow-wf]

I pulled in both, I believe redraws due to store triggers are being handled correctly, the code looks good to go.

I was just mentioning that I'm seeing some issues specifically on our stuff (we have a uiComponent that re-renders a bunch, and since we aren't checking shouldBatchRedraw inside of shouldComponentUpdate anymore, it causes that component to redraw when before it didn't). So something like what is defined in SOX-6634 might still be useful if we want our re-renders to be even more restrictive.

[greglittlefield-wf]

Cool, thanks for the info!

[maxwellpeterson-wf]

+10

• Tests pass
• Kyle tried it in graph_app with success

[maxwellpeterson-wf]

QA +1

• Dev +1's
• Dev/QA +10 with detail of what was tested
• Tests created/updated
• All tests pass

@Workiva/release-management-pp

[rmconsole-wf]

+1 from RM

[johnbland-wf]

@kylemcmorrow-wf sounds like a PureComponent need. ;)