Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EuiDataGrid] track re-renders from hooks #4347

Conversation

VladLasitsa
Copy link
Contributor

Summary

Draft

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs
  • Added documentation
  • Checked Code Sandbox works for the any docs examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@chandlerprall
Copy link
Contributor

Thanks for taking this first pass! It's noisy in the current state but the benefit is there. Some thoughts -

Hook form

I'd love to see useDebugMemo take the full form of useMemo+name instead of returning a new function wrapping the hook, e.g. const value = useDebugMemo(nameString, hookFn, dependencies)

Reporting changed dependencies

Detecting which dependencies of a useMemo changed, similar to usePerformanceCheck, would be super useful in useDebugMemo

Performance timings don't seem reported correctly

I created a snapshot where I can see the EuiDataGridStart mark but I don't see the finish one coming through, nor the measurement in the timeline. The measure does appear logged to the console and that object includes a duration, but for some reason I can't find it in the timeline?

Lodash

We're trying to migrate off any dependencies on Lodash - fine to include it for now while figuring out exactly how we want things to work, but we'll want to avoid it if reasonably possible in the end.

@chandlerprall
Copy link
Contributor

Awesome! This is already showing some places that could be addressed better. I still don't think the performance mark/measure is working quite as expected, and it isn't adding much value to the output (either console nor performance devtools). Let's remove the usePerformanceCheck function and its use. Remaining items:

  • turn the debugging output off by default, let it be enabled via a prop on EuiDataGrid - use context to set the debugging state & read from that in the new hooks
  • it would be fantastic if that debug prop could optionally specify what hook(s) to monitor, to help "zoom in" on specific issues without noise
  • linting cleanup (yarn lint-es)

@alexwizp
Copy link
Contributor

@chandlerprall what are we going to do with this PL? Do you still need that changes? If yes probably it make sense to update the description and close checklist items

@chandlerprall
Copy link
Contributor

@alexwizp 👍 thanks for the ping! I just brought it up with @flash1293 who is going to take a look at using this to help debug initial render/mount performance in the datagrid.

I can own any further changes to this PR, especially as there's likely conflicts with the virtualization branch. @VladLasitsa thank you very much for doing the initial work here! It's meaningful and I predict it will be very useful for other components as well.

@github-actions
Copy link

👋 Hey there. This PR hasn't had any activity for 90 days. We'll automatically close it if that trend continues for another week. If you feel this issue is still valid and needs attention please let us know with a comment.

@github-actions
Copy link

❌ We're automatically closing this PR due to lack of activity. Please comment if you feel this was done in error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants