perf(dashboard): reduce rerenders of DragDroppable

[kgabryje]

SUMMARY

Due to incorrect passing props to DragDroppable (i.e. passing inline functions and objects instead of memoizing them first), the component was being rerendered multiple times (1609 in the case of initial render of Sales Dashboard from sample data).
This PR improves that behaviour.
There's still a lot of room for improvement - in some cases it was problematic to memoize some functions without converting DragDroppable's parent from class to functional component. For that reason, there are still quite a lot of rerenders, especially in edit mode.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

I measured the number of rerenders of DragDroppable with whyDidYouRender library (https://github.com/welldone-software/why-did-you-render). It reports every rerender of a component that wasn't necessary and could've been avoided (e.g. by memoizing props or using PureComponent).

Before:
https://user-images.githubusercontent.com/15073128/131513340-9b1d96fc-052b-4f80-a7ca-44d116074c71.mov
After:
https://user-images.githubusercontent.com/15073128/131513453-44973062-5a48-4ef9-ad43-a1ee9ad8a96a.mov

TESTING INSTRUCTIONS

There should be no changes in how the dashboard works.

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

CC @junlincc

[codecov]

Codecov Report

Merging #16525 (f328b0d) into master (1f1e2dd) will increase coverage by 0.00%.
The diff coverage is 75.40%.

❗ Current head f328b0d differs from pull request most recent head 57e81e3. Consider uploading reports for the commit 57e81e3 to get more accurate results

@@           Coverage Diff           @@
##           master   #16525   +/-   ##
=======================================
  Coverage   76.71%   76.71%           
=======================================
  Files        1002     1002           
  Lines       53780    53807   +27     
  Branches     6859     6858    -1     
=======================================
+ Hits        41257    41280   +23     
- Misses      12286    12290    +4     
  Partials      237      237           

Flag	Coverage Δ
javascript	71.01% <67.64%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...end/src/dashboard/components/dnd/DragDroppable.jsx	94.59% <ø> (ø)
...ols/DndColumnSelectControl/utils/optionSelector.ts	46.15% <0.00%> (ø)
...d/src/dashboard/components/gridComponents/Tabs.jsx	86.60% <25.00%> (+0.12%)	⬆️
...rontend/src/dashboard/components/DashboardGrid.jsx	61.01% <33.33%> (+1.36%)	⬆️
superset/views/datasource/views.py	90.10% <50.00%> (-1.85%)	⬇️
...nd/src/dashboard/components/gridComponents/Tab.jsx	86.66% <66.66%> (+0.45%)	⬆️
...d/components/DashboardBuilder/DashboardBuilder.tsx	90.17% <94.11%> (+0.36%)	⬆️
superset/views/core.py	75.54% <95.23%> (+0.25%)	⬆️
superset/db_engine_specs/presto.py	89.95% <0.00%> (-0.42%)	⬇️

---

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 1f1e2dd...57e81e3. Read the comment docs.

[villebro]

Wow, incredible improvement again! I've said this before and I'll say this again - Given that we still don't have tightly enforced conventions in place for making sure that we can check for dashboardLayout in deps vs JSON.stringify(dashboardLayout), I expect us to hit some regression at some point by just checking if the object reference has changed. But in the long term I do feel we should aim to get rid of JSON.stringify in deps, so if we're confident this behavior works correctly in this case let's keep it this way for the sake of code cleanliness and perf (not sure how expensive JSON.stringify is, but I'm sure it adds up in a complex app like this).

[villebro]

This was here before, but wondering why this is here?

[kgabryje]

There are some typing errors returned by DashboardComponent. We should definitely fix them... in a separate PR 🙂
I fixed those weird white spaces around ts-ignore though

[kgabryje]

@villebro I don't see any JSON.stringify in the files modified by this PR. Do you mean any specific place, or using JSON.stringify in deps array in general? I agree that we should limit using that as much as possible - maybe we can do that gradually as a side effect of other future PRs?

[villebro]

@villebro I don't see any JSON.stringify in the files modified by this PR. Do you mean any specific place, or using JSON.stringify in deps array in general? I agree that we should limit using that as much as possible - maybe we can do that gradually as a side effect of other future PRs?

I merely meant that it was difficult for me to assess where it was sufficient to pass the object as-is in the deps array vs running it through JSON.stringify. I agree we should refrain fron stringifying whenever we can, and I also agree we should keep removal of stringification to future PRs. So no action needed here, just voicing my concerns/ignorance here 🙂