fix(chart_resizer): debounce resize set to leading

[nickofthyme]

Summary

Fix for ChartResizer initial render delay. A 200ms delay was applied to all initial renders.

Set debounce to leading to prevent initial load delay of 200ms.

Issue #109

Before

After

Checklist

• Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
• This was checked for cross-browser compatibility, including a check against IE11
• Proper documentation or storybook story was added for features that require explanation or tutorials
• Unit tests were updated or added to match the most common scenarios
• Each commit follows the convention

[codecov-io]

Codecov Report

Merging #229 into master will decrease coverage by 0.19%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master     #229     +/-   ##
=========================================
- Coverage   97.81%   97.61%   -0.2%     
=========================================
  Files          35       35             
  Lines        2564     2601     +37     
  Branches      561      578     +17     
=========================================
+ Hits         2508     2539     +31     
- Misses         49       55      +6     
  Partials        7        7

Impacted Files	Coverage Δ
src/state/crosshair_utils.ts	84% <0%> (-0.13%)	⬇️

---

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 efcde34...73666c6. Read the comment docs.

[rshen91]

I tested locally on Chrome. LGTM!

[Crazybus]

jenkins test this please

[markov00]

Code LGTM. Tested locally.
I've to configure a similar option also on #208 because we removed lodash/debounce in favour of ts-debouce

edit: I've found something that we may have to consider

[markov00]

On my previous review I considered the fixed behaviour a correct one, but I was missing one major thing:
using the leading option, the resize is triggered as soon as we receive a new resize event. This works fine for the first time rendering but it's not a good thing for subsequent resize calls.

The problem happen when you resize the container or the window: a resize will be triggered again on a slightly different parent size (maybe just a fraction of pixels when resizing the window) and than will be called again when the the resize ended (not sure why this happens, because if the leading property works as from the docs the function should be only triggered during on the first call).
This behaviour will trigger two times the computeChart function, that is something we don't want, mostly because retriggering the rendering for just a few pixel change doesn't have sense and because triggering 2 rendering can be expensive if we have chart with a high number of elements.

Instead, I think we can prefer a different approach:
the ResizeObserver listener function should check if we already received a first resize callback. If it's the first time, fire immediately, if not, use the debounce as normal (fire only at 200ms interval between calls)
What do you think?

I've found this issue because I was trying to apply the same leading option to ts-debounce the substitute library I'm using in a different PR. That library has a property call isImmediate that does exactly the same as leading fire at the beginning of the waiting time, but doesn't fire at the end :(

[nickofthyme]

@markov00 Ya I see your point. I was thinking this solution was too good to be true. I'll try your suggestion.

[nickofthyme]

@markov00 Your suggestion worked! The implementation doesn't seem so elegant with having a state variable to track the first render and a separate function to handle the resize. If you have any other ideas of how to implement it better let me know. I think if this comes up more we may want to write our own debounce function to have this option.

Demo

For some reason, resizing the browser window sometimes, not always, call the computeChart method twice still. Any ideas? Seems to just be my computer struggling to keep up.

Also to your point about calling computeChart twice. I noticed that on initial load or refresh that method is called twice. See console traces below 👇. But only the second call, the one from onResize, renders the chart to the page. Is that expected?

First call to `computeChart` looks to be from specs_parser

push../src/state/chart_state.ts.ChartStore.computeChart	@	chart_state.ts:757
push../src/specs/specs_parser.tsx.SpecsSpecRootComponent.componentDidMount	@	specs_parser.tsx:17
commitLifeCycles	@	react-dom.development.js:17334
commitAllLifeCycles	@	react-dom.development.js:18736
callCallback	@	react-dom.development.js:149
invokeGuardedCallbackDev	@	react-dom.development.js:199
invokeGuardedCallback	@	react-dom.development.js:256
commitRoot	@	react-dom.development.js:18948
(anonymous)	@	react-dom.development.js:20418
unstable_runWithPriority	@	scheduler.development.js:255
completeRoot	@	react-dom.development.js:20417
performWorkOnRoot	@	react-dom.development.js:20346
performWork	@	react-dom.development.js:20254
performSyncWork	@	react-dom.development.js:20228
requestWork	@	react-dom.development.js:20097
scheduleWork	@	react-dom.development.js:19911
scheduleRootUpdate	@	react-dom.development.js:20572
updateContainerAtExpirationTime	@	react-dom.development.js:20600
updateContainer	@	react-dom.development.js:20657
push../node_modules/react-dom/cjs/react-dom.development.js.ReactRoot.render	@	react-dom.development.js:20953
(anonymous)	@	react-dom.development.js:21090
unbatchedUpdates	@	react-dom.development.js:20459
legacyRenderSubtreeIntoContainer	@	react-dom.development.js:21086
render	@	react-dom.development.js:21155
render	@	render.js:51
renderMain	@	render.js:89
renderMain	@	start.js:208
renderUI	@	start.js:228
dispatch	@	redux.js:220
_renderMain	@	config_api.js:98
render	@	config_api.js:42
(anonymous)	@	config_api.js:71
(anonymous)	@	config.ts:48
./.storybook/config.ts	@	config.ts:48
__webpack_require__	@	bootstrap:782
fn	@	bootstrap:150
0	@	styling.tsx:673
__webpack_require__	@	bootstrap:782
checkDeferredModules	@	bootstrap:45
webpackJsonpCallback	@	bootstrap:32
(anonymous)	@	main.2dbf464….bundle.js:1

Second call to `computeChart` looks to be from chart_resizer

chart_state.ts:757 computeChart
push../src/state/chart_state.ts.ChartStore.computeChart	@	chart_state.ts:757
push../src/state/chart_state.ts.ChartStore.updateParentDimensions	@	chart_state.ts:716
(anonymous)	@	chart_resizer.tsx:30
Resizer._this.onResize	@	chart_resizer.tsx:29
invokeFunc	@	debounce.js:95
trailingEdge	@	debounce.js:144
timerExpired	@	debounce.js:132
setTimeout (async)		
leadingEdge	@	debounce.js:103
debounced	@	debounce.js:172

[markov00]

@nickofthyme I'm also not sure that this will be the right way to solve. Specially I'm not sure if we really need to use the react state for that usecase. I think we should simply use a private class variable to store that flag. This will avoid a re-rendering the component because of the state change.

Yes, you are right about the chart component that is called few times at the beginning. We have to clean that behaviour once we are in a good shape with the bug/feature requests. For the moment that method is called few times, but the real computation happens only when we have all the required paramenters to proceed (parent width, height, all the series etc). We will track this issue here: #75

[nickofthyme]

@markov00 Ya true not sure why I threw that in state. I replaced the state variable with a local private variable. Is there another way you thought of doing it entirely? Or is this approach ok?

[markov00]

@nickofthyme I think it's good to merge. I've took a look at the codecov and we actually don't have any test on that component so we can go on and merge right now.
Please squash before merge, clean the squash message and description to be aligned with the conventionalcommit standard.

[markov00]

🎉 This PR is included in version 4.2.9 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀