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

Fix SearchBoxBase leak by disposing EventGroup instance on unmount #11806

Merged
merged 3 commits into from
Jan 26, 2020
Merged

Fix SearchBoxBase leak by disposing EventGroup instance on unmount #11806

merged 3 commits into from
Jan 26, 2020

Conversation

KevinTCoughlin
Copy link
Member

@KevinTCoughlin KevinTCoughlin commented Jan 26, 2020 •
edited
Loading

Pull request checklist

  • Addresses an existing issue: Fixes #0000
  • Include a change request file using $ yarn change

Description of changes

Fix memory leak of SearchBoxBase by disposing of EventGroup instance in componentWillUnmount.

image

Relevant lines for EventGroup parent retainer:

https://github.com/OfficeDev/office-ui-fabric-react/blob/9e7b65719ad144fcbce97850140b0c703378d26e/packages/utilities/src/EventGroup.ts#L163

https://github.com/OfficeDev/office-ui-fabric-react/blob/9e7b65719ad144fcbce97850140b0c703378d26e/packages/utilities/src/EventGroup.ts#L172

Focus areas to test

  • CI should pass.
  • To verify, conditionally render the control and capture comparison heap snapshots.
Microsoft Reviewers: Open in CodeFlow

@dzearing
Copy link
Member

Good catch!!!

@msft-github-bot
Copy link
Contributor

Component Perf Analysis

No significant results to display.

All results

Scenario Master Ticks PR Ticks Status
BaseButton 717 680
BaseButton (experiments) 916 941
DefaultButton 966 1001
DefaultButton (experiments) 1805 1800
DetailsRow 3206 3283
DetailsRow (fast icons) 3230 3280
DetailsRow without styles 2967 2997
DocumentCardTitle with truncation 1606 1582
MenuButton 1281 1268
MenuButton (experiments) 3463 3364
PrimaryButton 1180 1137
PrimaryButton (experiments) 1914 1893
SplitButton 2815 2737
SplitButton (experiments) 6758 6823
Stack 450 439
Stack with Intrinsic children 1100 1061
Stack with Text children 3962 3970
Text 354 354
Toggle 781 801
Toggle (experiments) 2167 2155
button 69 59

@size-auditor
Copy link

size-auditor bot commented Jan 26, 2020

Asset size changes

Project Bundle Baseline Size New Size Difference
office-ui-fabric-react SearchBox 174.23 kB 174.225 kB BelowBaseline     -5 bytes

ExceedsTolerance Over Tolerance (1024 B) ExceedsBaseline Over Baseline BelowBaseline Below Baseline New New Deleted  Removed 1 kB = 1000 B

Baseline commit: 9e7b65719ad144fcbce97850140b0c703378d26e (build)

@dzearing dzearing merged commit 3c746c8 into microsoft:master Jan 26, 2020
@msft-github-bot
Copy link
Contributor

🎉office-ui-fabric-react@v7.85.0 has been released which incorporates this pull request.:tada:

Handy links:

@KevinTCoughlin KevinTCoughlin deleted the keco/searchbox-leak branch January 27, 2020 16:27
@KevinTCoughlin KevinTCoughlin mentioned this pull request Feb 14, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dzearing dzearing dzearing approved these changes

@ecraig12345 ecraig12345 Awaiting requested review from ecraig12345

@qianghuang1 qianghuang1 Awaiting requested review from qianghuang1

Assignees

@khmakoto khmakoto

Labels
Component: SearchBox
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@KevinTCoughlin @dzearing @msft-github-bot @khmakoto