Skip to content

Conversation

@shahzad31
Copy link
Contributor

@shahzad31 shahzad31 commented Feb 10, 2022

Summary

Fixes #125200

Wrap bucketsize value in useMemo, since it was triggering unnecessary re-renders.

@shahzad31 shahzad31 marked this pull request as ready for review February 10, 2022 09:55
@shahzad31 shahzad31 requested a review from a team as a code owner February 10, 2022 09:55
@shahzad31 shahzad31 self-assigned this Feb 10, 2022
@shahzad31 shahzad31 added v8.1.0 v8.2.0 release_note:skip Skip the PR/issue when compiling release notes labels Feb 10, 2022
@kibana-ci
Copy link

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
observability 393.0KB 393.2KB +180.0B

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @shahzad31

Comment on lines +62 to +75
const bucketSize = calculateBucketSize({
start: absoluteTime.start,
end: absoluteTime.end,
});

const bucketSizeValue = useMemo(() => {
if (bucketSize?.bucketSize) {
return {
bucketSize: bucketSize.bucketSize,
intervalString: bucketSize.intervalString,
};
}
}, [bucketSize?.bucketSize, bucketSize?.intervalString]);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that useMemo is the best option here. bucketSize and bucketSizeValue are exactly the same value and I don't think it reflects the intentions of what we want to achieve, which could lead to confusion and bugs in the future.
bucketSize should probably be part of the state of the component and we should only update it when absoluteTime.start or absoluteTime.end change, preventing unnecessary rerenders.

What do you think about something like this:

Suggested change
const bucketSize = calculateBucketSize({
start: absoluteTime.start,
end: absoluteTime.end,
});
const bucketSizeValue = useMemo(() => {
if (bucketSize?.bucketSize) {
return {
bucketSize: bucketSize.bucketSize,
intervalString: bucketSize.intervalString,
};
}
}, [bucketSize?.bucketSize, bucketSize?.intervalString]);
const [bucketSize, setBucketSize] = useState(undefined);
useEffect(() => {
const bucket = calculateBucketSize({
start: absoluteTime.start,
end: absoluteTime.end,
});
setBucketSize(bucket);
}, [absoluteTime.start, absoluteTime.end])

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think absolute time start and end changes whenever component re-renders , so can't use those in side effect

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see... I wasn't expecting it 🙃

@shahzad31 shahzad31 merged commit 63dffb1 into elastic:main Feb 11, 2022
@shahzad31 shahzad31 deleted the improve-dependency branch February 11, 2022 08:31
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 125204 or prevent reminders by adding the backport:skip label.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 15, 2022
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 125204 or prevent reminders by adding the backport:skip label.

@shahzad31 shahzad31 added the backport:skip This PR does not require backporting label Feb 16, 2022
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting release_note:skip Skip the PR/issue when compiling release notes v8.1.0 v8.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Observability overview page is slow because of redundant/duplicate requests

4 participants