Skip to content

Conversation

@bsunderhus
Copy link
Contributor

@bsunderhus bsunderhus commented Apr 27, 2023

New Behavior

  1. defers initialization of internal state of the useControllableState hook to the init method provided by useState hook

@fabricteam
Copy link
Collaborator

fabricteam commented Apr 27, 2023

📊 Bundle size report

Package & Exports Baseline (minified/GZIP) PR Change
react-accordion
Accordion (including children components)
86.012 kB
26.086 kB
86.064 kB
26.1 kB
52 B
14 B
react-button
ToggleButton
54.814 kB
11.358 kB
54.866 kB
11.367 kB
52 B
9 B
react-checkbox
Checkbox
34.216 kB
10.784 kB
34.268 kB
10.793 kB
52 B
9 B
react-combobox
Combobox (including child components)
87.199 kB
28.095 kB
87.251 kB
28.1 kB
52 B
5 B
react-combobox
Dropdown (including child components)
85.583 kB
27.692 kB
85.635 kB
27.697 kB
52 B
5 B
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
203.885 kB
57.081 kB
203.937 kB
57.086 kB
52 B
5 B
react-datepicker-compat
DatePicker Compat
220.251 kB
58.471 kB
220.303 kB
58.486 kB
52 B
15 B
react-dialog
Dialog (including children components)
90.867 kB
27.047 kB
90.919 kB
27.052 kB
52 B
5 B
react-infobutton
InfoButton
127.925 kB
39.003 kB
127.977 kB
39.012 kB
52 B
9 B
react-infobutton
InfoLabel
131.208 kB
39.996 kB
131.26 kB
40 kB
52 B
4 B
react-input
Input
23.972 kB
7.676 kB
24.024 kB
7.682 kB
52 B
6 B
react-menu
Menu (including children components)
128.202 kB
39.18 kB
128.254 kB
39.184 kB
52 B
4 B
react-menu
Menu (including selectable components)
131.186 kB
39.696 kB
131.238 kB
39.7 kB
52 B
4 B
react-popover
Popover
114.917 kB
35.388 kB
114.969 kB
35.392 kB
52 B
4 B
react-slider
Slider
34.112 kB
11.018 kB
34.164 kB
11.024 kB
52 B
6 B
react-spinbutton
SpinButton
33.882 kB
10.325 kB
33.934 kB
10.328 kB
52 B
3 B
react-table
DataGrid
147.531 kB
40.517 kB
147.583 kB
40.523 kB
52 B
6 B
react-table
Table as DataGrid
130.09 kB
33.08 kB
130.142 kB
33.083 kB
52 B
3 B
react-table
Table (Selection only)
78.004 kB
19.079 kB
78.056 kB
19.084 kB
52 B
5 B
react-table
Table (Sort only)
77.334 kB
18.891 kB
77.386 kB
18.896 kB
52 B
5 B
react-textarea
Textarea
27.399 kB
9.041 kB
27.451 kB
9.046 kB
52 B
5 B
react-tooltip
Tooltip
46.656 kB
16.369 kB
46.708 kB
16.376 kB
52 B
7 B
Unchanged fixtures
Package & Exports Size (minified/GZIP)
global-context
createContext
533 B
341 B
global-context
createContextSelector
560 B
352 B
react-alert
Alert
93.534 kB
22.497 kB
react-avatar
Avatar
57.754 kB
15.041 kB
react-avatar
AvatarGroup
15.632 kB
6.258 kB
react-avatar
AvatarGroupItem
73.968 kB
19.552 kB
react-badge
Badge
23.512 kB
7.197 kB
react-badge
CounterBadge
24.416 kB
7.506 kB
react-badge
PresenceBadge
32.094 kB
8.367 kB
react-button
Button
36.724 kB
9.458 kB
react-button
CompoundButton
43.873 kB
10.939 kB
react-button
MenuButton
41.411 kB
10.791 kB
react-button
SplitButton
49.635 kB
12.364 kB
react-card
Card - All
86.061 kB
24.345 kB
react-card
Card
80.997 kB
22.895 kB
react-card
CardFooter
9.158 kB
3.844 kB
react-card
CardHeader
11.048 kB
4.538 kB
react-card
CardPreview
9.963 kB
4.192 kB
react-components
react-components: Button, FluentProvider & webLightTheme
64.855 kB
17.852 kB
react-components
react-components: FluentProvider & webLightTheme
36.086 kB
11.9 kB
react-divider
Divider
17.399 kB
6.298 kB
react-field
Field
18.864 kB
7.004 kB
react-image
Image
11.479 kB
4.573 kB
react-label
Label
10.104 kB
4.185 kB
react-link
Link
12.304 kB
5.061 kB
react-overflow
hooks only
11.214 kB
4.271 kB
react-persona
Persona
64.675 kB
16.968 kB
react-portal
Portal
11.649 kB
4.262 kB
react-portal-compat
PortalCompatProvider
6.446 kB
2.186 kB
react-positioning
usePositioning
24.008 kB
8.798 kB
react-progress
ProgressBar
13.856 kB
5.434 kB
react-provider
FluentProvider
18.033 kB
6.666 kB
react-radio
Radio
27.282 kB
8.661 kB
react-radio
RadioGroup
11.312 kB
4.71 kB
react-select
Select
25.357 kB
8.798 kB
react-spinner
Spinner
20.882 kB
6.798 kB
react-switch
Switch
29.806 kB
9.274 kB
react-table
Table (Primitives only)
44.348 kB
12.347 kB
react-tags
Tag
21.687 kB
7.868 kB
react-text
Text - Default
12.492 kB
4.92 kB
react-text
Text - Wrappers
15.624 kB
5.232 kB
react-utilities
SSRProvider
180 B
159 B
🤖 This report was generated against 3a9044346aced02d96e8cde65b46e88e8d9d32ec

@fabricteam
Copy link
Collaborator

fabricteam commented Apr 27, 2023

Perf Analysis (@fluentui/react-components)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 697 704 5000
Button mount 372 369 5000
Field mount 1257 1259 5000
FluentProvider mount 886 887 5000
FluentProviderWithTheme mount 107 114 10
FluentProviderWithTheme virtual-rerender 94 90 10
FluentProviderWithTheme virtual-rerender-with-unmount 98 110 10
InfoButton mount 16 18 5000
MakeStyles mount 1105 1135 50000
Persona mount 1993 1997 5000
SpinButton mount 1544 1566 5000

@size-auditor
Copy link

size-auditor bot commented Apr 27, 2023

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: 3a9044346aced02d96e8cde65b46e88e8d9d32ec (build)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 27, 2023

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.

Latest deployment of this branch, based on commit ef642a9:

Sandbox Source
@fluentui/react 8 starter Configuration
@fluentui/react-components 9 starter Configuration

@fabricteam
Copy link
Collaborator

fabricteam commented Apr 27, 2023

🕵 fluentuiv9 No visual regressions between this PR and main

@bsunderhus bsunderhus force-pushed the react-utilities/feat--useControllableState-hook-improvements branch 3 times, most recently from 247a5d2 to 9cce48e Compare April 28, 2023 08:16
@bsunderhus bsunderhus changed the title feat(react-utilities): useControllableState hook improvements feat(react-utilities): useControllableState hook simplification Apr 28, 2023
@bsunderhus bsunderhus force-pushed the react-utilities/feat--useControllableState-hook-improvements branch from 9cce48e to 3a998b0 Compare May 3, 2023 07:17
@bsunderhus bsunderhus force-pushed the react-utilities/feat--useControllableState-hook-improvements branch 3 times, most recently from 382b813 to e1aca8c Compare May 3, 2023 11:59
@layershifter
Copy link
Member

Note: it's a breaking change 🚨 The hook is not re-exported from @fluentui/react-components, but it's used in multiple products directly.


Modifies hook signature by moving initializer function from defaultState to initialState

The whole thing behind autocontrolled pattern is hard to understand for people who are not familiar with it. The separation in current implementation was intentional:

  • state is a controlled value i.e. props.open
  • defaultState is a default uncontrolled value i.e. props.defaultProps
  • initialState is a fallback value from both are not provided to prevent undefined to be used as a return value

While it's nice to have less concepts, IMO current change does not make that difference more obvious to components' authors.

@bsunderhus
Copy link
Contributor Author

Note: it's a breaking change 🚨 The hook is not re-exported from @fluentui/react-components, but it's used in multiple products directly.

Modifies hook signature by moving initializer function from defaultState to initialState

The whole thing behind autocontrolled pattern is hard to understand for people who are not familiar with it. The separation in current implementation was intentional:

  • state is a controlled value i.e. props.open
  • defaultState is a default uncontrolled value i.e. props.defaultProps
  • initialState is a fallback value from both are not provided to prevent undefined to be used as a return value

While it's nice to have less concepts, IMO current change does not make that difference more obvious to components' authors.

I see your point and I kind of agree, although I like the simplification I can see where this might become inconvenient for some people. I'll update the PR to return initialState idea with some minor changes.

@bsunderhus bsunderhus force-pushed the react-utilities/feat--useControllableState-hook-improvements branch 2 times, most recently from 9d8cd43 to 126881b Compare May 3, 2023 13:23
@bsunderhus bsunderhus marked this pull request as ready for review May 3, 2023 13:25
@bsunderhus bsunderhus requested review from a team and smhigley as code owners May 3, 2023 13:25
@ling1726
Copy link
Contributor

ling1726 commented May 3, 2023

Breaking change

This hook has been annotated with @internal since the first release, we should not remove it but, but IMO breaking is fine. We have been as explicit as it gets with this one

Removing initial state

Yeah we should not remove this. It's really important for our auto-controlled scenarios because we effectively drill down the user's defaultValue to this hook. In most auto controlled cases the user will not provide a defaultValue and we have no way to make the typings to make defaultValue required if value is not set.

actually yeah thinking about it some more, it could be done

ling1726
ling1726 previously approved these changes May 3, 2023
@bsunderhus bsunderhus force-pushed the react-utilities/feat--useControllableState-hook-improvements branch from 126881b to d185ce6 Compare May 3, 2023 14:00
@bsunderhus bsunderhus requested review from a team, behowell and khmakoto as code owners May 3, 2023 14:00
@bsunderhus bsunderhus force-pushed the react-utilities/feat--useControllableState-hook-improvements branch 2 times, most recently from f5619c6 to 83af1b5 Compare May 3, 2023 14:19
@bsunderhus bsunderhus requested a review from sopranopillow as a code owner May 3, 2023 14:19
@bsunderhus bsunderhus force-pushed the react-utilities/feat--useControllableState-hook-improvements branch 2 times, most recently from 5cd12d0 to 11eb9ba Compare May 3, 2023 15:00
@ling1726 ling1726 dismissed their stale review May 4, 2023 09:55

Needs discussion on breaking change

@bsunderhus bsunderhus force-pushed the react-utilities/feat--useControllableState-hook-improvements branch from 11eb9ba to c822351 Compare May 10, 2023 11:59
@bsunderhus bsunderhus force-pushed the react-utilities/feat--useControllableState-hook-improvements branch from c822351 to 3022be8 Compare May 10, 2023 12:01
@bsunderhus bsunderhus changed the title feat(react-utilities): useControllableState hook simplification feat: defers useControllableState state to initializer method May 10, 2023
@bsunderhus bsunderhus requested a review from ling1726 May 10, 2023 12:02
@bsunderhus bsunderhus force-pushed the react-utilities/feat--useControllableState-hook-improvements branch 2 times, most recently from b79e82f to 8b0e7e3 Compare May 10, 2023 13:46
@bsunderhus bsunderhus requested a review from layershifter May 10, 2023 13:46
@bsunderhus bsunderhus force-pushed the react-utilities/feat--useControllableState-hook-improvements branch from 8b0e7e3 to ef642a9 Compare May 10, 2023 13:49
@bsunderhus bsunderhus merged commit 88ff00c into microsoft:master May 11, 2023
@bsunderhus bsunderhus deleted the react-utilities/feat--useControllableState-hook-improvements branch May 11, 2023 09:15
marcosmoura added a commit to marcosmoura/fluentui that referenced this pull request May 12, 2023
* feat/drawer-header:
  feat: Update existing toasts (microsoft#27827)
  bugfix: fix horizontal overflow on tree (microsoft#27825)
  feat: Allow toast options to be configured from the toaster (microsoft#27820)
  feat: creates TreeItemAside component (microsoft#27701)
  fix: generate API
  fix: use composition to build DrawerHeaderTitle
  Virtualizer: Dynamic scroll view and optimizations (microsoft#27298)
  bugfix(react-dialog): change DialogTitle default action icon size (microsoft#27815)
  feat: Implement Toast pause (microsoft#27811)
  fix: ToolbarToggleButton should not follow Toolbar size (microsoft#27797)
  feat: defers useControllableState state to initializer method (microsoft#27717)
  feat: Implement toast dismiss (microsoft#27810)
@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

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.

6 participants