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.

Sign up for GitHub

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

Jump to bottom

[EuiComboBox] Replace autosizing input dependency with more performant utility #7215

Merged
merged 6 commits into from
Sep 26, 2023
Merged

[EuiComboBox] Replace autosizing input dependency with more performant utility #7215

merged 6 commits into from
Sep 26, 2023

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Sep 21, 2023 •
edited
Loading

Summary

closes #4866, and incidentally partially (but not completely) addresses #7080

When I was upgrading react-autosizer-input with React 18 support, I started grepping through to see usages and I was like... "Hmm, it's just one usage just for EuiComboBox? This isn't a hard problem to solve, surely we could just roll our own instead..." and it turns out not only can we Do It Ourselves, we can utilize our new canvas rendering text utils to do it way more performantly 🎉

ℹ️ Quick note, this PR has extra reported diffs simply due to spacing changes - I'd recommend viewing diffs with whitespace hidden.

QA

General checklist

  • Browser QA
    • Checked in both light and dark modes
    • Checked in mobile
    • Checked in Chrome, Safari, Edge, and Firefox
    • Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA - N/A
  • Code quality checklist
  • Release checklist
    • A changelog entry exists and is marked appropriately.

@cee-chen cee-chen force-pushed the remove-react-input-autosize branch from 4667411 to 9a474b3 Compare September 21, 2023 22:16
@cee-chen
Copy link
Member Author

@dej611 I thought you might get a kick out of this PR - as it turns out, the new canvas text utilities you helped add for truncation can be reused to replace the AutosizeInput dependency that you reported as causing perf issues a while back :) If you have some time this week, we'd super appreciate you taking a look at staging and confirming if the rerender issues are resolved, but no worries if not!

@cee-chen cee-chen marked this pull request as ready for review September 21, 2023 22:25
@cee-chen cee-chen requested a review from a team as a code owner September 21, 2023 22:25
@cee-chen cee-chen requested a review from breehall September 21, 2023 22:25
@cee-chen cee-chen force-pushed the remove-react-input-autosize branch 3 times, most recently from 9ebc022 to ca66578 Compare September 25, 2023 20:11
@cee-chen cee-chen force-pushed the remove-react-input-autosize branch from ca66578 to adcc501 Compare September 25, 2023 20:35
@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@cee-chen
Copy link
Member Author

@breehall @1Copenut This is ready for review whenever!

@dej611
Copy link
Contributor

dej611 commented Sep 26, 2023

I've ran some tests with this EUI optimization vs current Kibana main with the React profiler again: no substantial changes were detected in terms of rendering timing, but also it didn't get any worse.
Number are still around the same value for the use case, maybe the resizer got better since I've reported the initial performance issue? Anyway I see this as an improvement as the measuring logic is now fully controlled and one less dependency to think about! 🎉

breehall
breehall approved these changes Sep 26, 2023
View reviewed changes
Copy link
Contributor

@breehall breehall left a comment

Choose a reason for hiding this comment

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

It's always nice to be able to replace an dependency with internal code! Tested by creating long text additions in staging and in prod, and didn't find any differences. I wasn't able to notice a significant performance increase by eye, but there was no delay either (which is a win!)
I only had one comment in regards to naming below, and then this is good to go!

src/components/text_truncate/utils.testenv.ts
import { TruncationUtils as _TruncationUtils } from './utils';

export class CanvasTextUtils {
constructor(_: any) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it would be helpful to provide an name for this parameter, rather than _? Or are we using this as a standard?

Copy link
Member Author

Choose a reason for hiding this comment

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

_ is generally the standard to indicate that a variable is unused but is present for some reason (e.g., in this case I'm adding it so that any test mock usages of it don't throw annoying type errors that are dissimilar to non-mocked class).

You'll notice in VSCode that when a named param is unused, vscode will fade it out and eslint may complain that it's unused, whereas when you rename it to _ it becomes unfaded and typically eslint will stop complaining.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok! Thank you for the explanation!

upcoming_changelogs/7215.md
@@ -0,0 +1 @@
- Improved the performance of `EuiComboBox` by removing the `react-autosizer-input` dependency
Copy link
Contributor

Choose a reason for hiding this comment

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

🎊

@cee-chen
Copy link
Member Author

I've ran some tests with this EUI optimization vs current Kibana main with the React profiler again: no substantial changes were detected in terms of rendering timing, but also it didn't get any worse.

Thanks so much for checking Marco! I'll take "the same performance" over "worse performance" any day :)

Just to clarify, it looks like when you reported the issue in #4866 you mentioned that you had to add a bunch of optimizations to reduce the number of rerenders of the EuiComboBox component. Just curious, can you now remove those optimizations and the perf remains the same?

@cee-chen cee-chen merged commit fedc004 into elastic:main Sep 26, 2023
1 check passed
@cee-chen cee-chen deleted the remove-react-input-autosize branch September 26, 2023 15:29
This was referenced Sep 28, 2023
Merged
Merged
Merged
cee-chen added a commit to elastic/kibana that referenced this pull request Oct 4, 2023
@cee-chen
Upgrade EUI to v88.5.4 (#167555)
038ac89 
`v88.5.0`⏩`v88.5.4`

This EUI upgrade helps unblock the Shared UX team with some beta
serverless nav updates not listed in the below changelog
(elastic/eui#7228 and
elastic/eui#7248).

---

## [`88.5.4`](https://github.com/elastic/eui/tree/v88.5.4)

- This release contains internal changes to a beta component needed by
Kibana.

## [`88.5.3`](https://github.com/elastic/eui/tree/v88.5.3)

**Bug fixes**

- Fixed `EuiComboBox` search input width not resetting correctly on
selection ([#7240](elastic/eui#7240))

## [`88.5.2`](https://github.com/elastic/eui/tree/v88.5.2)

**Bug fixes**

- Fixed broken `EuiTextTruncate` testenv mocks
([#7234](elastic/eui#7234))

## [`88.5.1`](https://github.com/elastic/eui/tree/v88.5.1)

- Improved the performance of `EuiComboBox` by removing the
`react-autosizer-input` dependency
([#7215](elastic/eui#7215))

**Dependency updates**

- Updated `react-element-to-jsx-string` to v5.0.0
([#7214](elastic/eui#7214))
- Removed unused `@types/vfile-message` dependency
([#7214](elastic/eui#7214))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@breehall breehall breehall approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[EuiComboBox] Performance issue on rerender
5 participants
@cee-chen @kibanamachine @elasticmachine @dej611 @breehall