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

fix: replace internal useKeyborg() with useKeyborgRef() #31295

Merged
merged 1 commit into from
May 7, 2024

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented May 7, 2024

Previous Behavior

What was happening?

React 18, no StrictMode

- render, create `Keyborg` in `useMemo()` #1
- unmount
- dispose `Keyborg` in `useEffect()` #1

React 18, StrictMode

- render, create `Keyborg` in `useMemo()` #1
- render, create `Keyborg` in `useMemo()` #2
- dispose `Keyborg` in `useEffect()` #2
- unmount
- dispose `Keyborg` in `useEffect()` #2

Note: The problem is described in detail in https://github.com/microsoft/use-disposable.

New Behavior

Keyborg instances are created in useEffect now and are disposed properly.

Related Issue(s)

@github-actions github-actions bot added this to the April Project Cycle Q1 2024 milestone May 7, 2024
Copy link

codesandbox-ci bot commented May 7, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@fabricteam
Copy link
Collaborator

Perf Analysis (@fluentui/react-components)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 668 630 5000
Button mount 308 302 5000
Field mount 1129 1151 5000
FluentProvider mount 706 706 5000
FluentProviderWithTheme mount 71 84 10
FluentProviderWithTheme virtual-rerender 32 33 10
FluentProviderWithTheme virtual-rerender-with-unmount 73 83 10
MakeStyles mount 847 894 50000
Persona mount 1758 1727 5000
SpinButton mount 1423 1433 5000
SwatchPicker mount 1522 1557 5000

@fabricteam
Copy link
Collaborator

📊 Bundle size report

Package & Exports Baseline (minified/GZIP) PR Change
react-combobox
Combobox (including child components)
105.464 kB
33.885 kB
105.556 kB
33.9 kB
92 B
15 B
react-combobox
Dropdown (including child components)
106.936 kB
33.86 kB
107.028 kB
33.875 kB
92 B
15 B
react-components
react-components: entire library
1.101 MB
267.249 kB
1.101 MB
267.271 kB
92 B
22 B
react-tag-picker-preview
@fluentui/react-tag-picker-preview - package
188.87 kB
55.778 kB
188.962 kB
55.803 kB
92 B
25 B
react-timepicker-compat
TimePicker
107.482 kB
35.234 kB
107.574 kB
35.25 kB
92 B
16 B
Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-accordion
Accordion (including children components)
101.386 kB
30.643 kB
react-alert
Alert
83.737 kB
23.475 kB
react-avatar
Avatar
50.175 kB
15.944 kB
react-avatar
AvatarGroup
19.702 kB
7.794 kB
react-avatar
AvatarGroupItem
64.829 kB
20.272 kB
react-breadcrumb
@fluentui/react-breadcrumb - package
117.263 kB
32.241 kB
react-button
Button
39.513 kB
11.17 kB
react-button
CompoundButton
46.874 kB
12.662 kB
react-button
MenuButton
44.292 kB
12.544 kB
react-button
SplitButton
52.306 kB
14.135 kB
react-button
ToggleButton
56.558 kB
13.068 kB
react-calendar-compat
Calendar Compat
153.874 kB
40.203 kB
react-card
Card - All
104.438 kB
29.41 kB
react-card
Card
97.449 kB
27.681 kB
react-card
CardFooter
13.971 kB
5.626 kB
react-card
CardHeader
16.214 kB
6.386 kB
react-card
CardPreview
14.015 kB
5.752 kB
react-checkbox
Checkbox
36.102 kB
12.131 kB
react-components
react-components: Button, FluentProvider & webLightTheme
71.55 kB
20.584 kB
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
221.825 kB
62.568 kB
react-components
react-components: FluentProvider & webLightTheme
44.037 kB
14.418 kB
react-datepicker-compat
DatePicker Compat
227.989 kB
63.592 kB
react-dialog
Dialog (including children components)
117.345 kB
36.161 kB
react-link
Link
17.082 kB
6.911 kB
react-menu
Menu (including children components)
154.383 kB
46.161 kB
react-menu
Menu (including selectable components)
157.069 kB
46.71 kB
react-message-bar
MessageBar (all components)
24.204 kB
8.983 kB
react-persona
Persona
57.066 kB
17.821 kB
react-popover
Popover
128.669 kB
40.26 kB
react-portal
Portal
14.163 kB
4.948 kB
react-portal-compat
PortalCompatProvider
8.39 kB
2.64 kB
react-provider
FluentProvider
24.211 kB
8.721 kB
react-radio
Radio
33.396 kB
10.316 kB
react-radio
RadioGroup
15.354 kB
6.265 kB
react-slider
Slider
40.395 kB
13.024 kB
react-swatch-picker-preview
@fluentui/react-swatch-picker-preview - package
108.862 kB
30.32 kB
react-switch
Switch
36.04 kB
11.305 kB
react-table
DataGrid
169.274 kB
46.907 kB
react-table
Table (Primitives only)
45.77 kB
14.174 kB
react-table
Table as DataGrid
138.358 kB
37.213 kB
react-table
Table (Selection only)
76.774 kB
20.602 kB
react-table
Table (Sort only)
75.417 kB
20.205 kB
react-tags
InteractionTag
15.299 kB
6.08 kB
react-tags
Tag
29.092 kB
9.418 kB
react-tags
TagGroup
82.49 kB
24.46 kB
react-toast
Toast (including Toaster)
99.113 kB
29.844 kB
react-tooltip
Tooltip
55.201 kB
19.285 kB
🤖 This report was generated against 2f4006afba4bdb5a9ff136a5ee8079bfd1533268

@fabricteam
Copy link
Collaborator

🕵 fluentuiv9 No visual regressions between this PR and main

@layershifter layershifter marked this pull request as ready for review May 7, 2024 11:16
@layershifter layershifter requested a review from a team as a code owner May 7, 2024 11:16
Copy link
Contributor

@bsunderhus bsunderhus left a comment

Choose a reason for hiding this comment

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

LGTM, I don't get it how simply moving the creation of keyborg instance from useMemo to useEffect solved this though.... is there a reason?

@layershifter
Copy link
Member Author

LGTM, I don't get it how simply moving the creation of keyborg instance from useMemo to useEffect solved this though.... is there a reason?

@bsunderhus I tried to explain it in PR description, but it seems that I failed. Let me try explain it better with code 🐱

Here is an example: https://stackblitz.com/edit/vitejs-vite-dae6we. We subscribed to notifications about keyboard nav state on a destroyed instance.

@layershifter layershifter merged commit fded6b4 into microsoft:master May 7, 2024
19 checks passed
@layershifter layershifter deleted the fix/keyborg-creation branch May 7, 2024 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants