Skip to content

Conversation

@bsunderhus
Copy link
Contributor

@bsunderhus bsunderhus commented Sep 23, 2022

Main reason for this split would only be better separation of concern. The way I see trigger has its own domain logic meanwhile react-utilities should be more devoted for general purpose things. I have the same feeling for slots (it should be separated from utilities), but I guess its already too late for that...

  1. Moves trigger methods from @fluentui/react-utilities to @fluentui/react-trigger
  2. Updates dependencies from @fluentui/react-utilities to @fluentui/react-trigger
    • react-dialog
    • react-menu
    • react-overflow
    • react-popover
    • react-tooltip

@fabricteam
Copy link
Collaborator

fabricteam commented Sep 23, 2022

📊 Bundle size report

Package & Exports Baseline (minified/GZIP) PR Change
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
188.681 kB
52.366 kB
188.675 kB
52.363 kB
-6 B
-3 B
react-menu
Menu (including children components)
116.589 kB
35.777 kB
116.583 kB
35.778 kB
-6 B
1 B
react-menu
Menu (including selectable components)
119.658 kB
36.296 kB
119.652 kB
36.296 kB
-6 B
react-popover
Popover
102.955 kB
31.548 kB
102.949 kB
31.546 kB
-6 B
-2 B
Unchanged fixtures
Package & Exports Size (minified/GZIP)
global-context
createContext
533 B
341 B
global-context
createContextSelector
554 B
348 B
react-accordion
Accordion (including children components)
78.914 kB
24.06 kB
react-alert
Alert
83.822 kB
21.029 kB
react-avatar
Avatar
48.692 kB
13.8 kB
react-avatar
AvatarGroup
14.95 kB
5.989 kB
react-avatar
AvatarGroupItem
68.66 kB
19.138 kB
react-badge
Badge
22.6 kB
7.205 kB
react-badge
CounterBadge
23.503 kB
7.497 kB
react-badge
PresenceBadge
24.05 kB
7.067 kB
react-button
Button
36.119 kB
9.647 kB
react-button
CompoundButton
43.144 kB
10.86 kB
react-button
MenuButton
38.813 kB
10.551 kB
react-button
SplitButton
46.228 kB
11.933 kB
react-button
ToggleButton
51.888 kB
11.127 kB
react-card
Card - All
67.002 kB
19.261 kB
react-card
Card
62.684 kB
18.177 kB
react-card
CardFooter
8.561 kB
3.601 kB
react-card
CardHeader
9.604 kB
3.94 kB
react-card
CardPreview
8.662 kB
3.656 kB
react-combobox
Combobox (including child components)
74.636 kB
24.186 kB
react-combobox
Dropdown (including child components)
74.236 kB
24.086 kB
react-components
react-components: FluentProvider & webLightTheme
33.394 kB
11.007 kB
react-dialog
Dialog (including children components)
82.755 kB
24.581 kB
react-divider
Divider
16.459 kB
5.902 kB
react-image
Image
10.78 kB
4.264 kB
react-input
Input
23.757 kB
7.704 kB
react-label
Label
9.338 kB
3.86 kB
react-link
Link
11.784 kB
4.867 kB
react-overflow
hooks only
10.685 kB
4.104 kB
react-portal
Portal
10.576 kB
3.875 kB
react-portal-compat
PortalCompatProvider
5.851 kB
1.964 kB
react-positioning
usePositioning
19.7 kB
7.404 kB
react-provider
FluentProvider
15.755 kB
5.883 kB
react-radio
Radio
35.56 kB
11.929 kB
react-radio
RadioGroup
14.248 kB
5.7 kB
react-select
Select
20.846 kB
7.346 kB
react-slider
Slider
31.526 kB
10.046 kB
react-spinbutton
SpinButton
44.102 kB
12.425 kB
react-spinner
Spinner
19.977 kB
6.438 kB
react-switch
Switch
32.097 kB
10.27 kB
react-text
Text - Default
11.782 kB
4.605 kB
react-text
Text - Wrappers
15.092 kB
5.044 kB
react-textarea
Textarea
25.013 kB
8.133 kB
react-tooltip
Tooltip
41.535 kB
14.639 kB
react-utilities
SSRProvider
180 B
159 B
🤖 This report was generated against e58b334fbe079a3679a797d7252d4409681698d8

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 23, 2022

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 60ee223:

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

@fabricteam
Copy link
Collaborator

fabricteam commented Sep 23, 2022

Perf Analysis (@fluentui/react-components)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 1321 1348 5000
Button mount 963 922 5000
FluentProvider mount 1593 1555 5000
FluentProviderWithTheme mount 633 632 10
FluentProviderWithTheme virtual-rerender 585 589 10
FluentProviderWithTheme virtual-rerender-with-unmount 626 636 10
MakeStyles mount 1898 1926 50000
SpinButton mount 2532 2570 5000

@size-auditor
Copy link

size-auditor bot commented Sep 23, 2022

Asset size changes

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

Baseline commit: e58b334fbe079a3679a797d7252d4409681698d8 (build)

@bsunderhus bsunderhus force-pushed the chore(react-trigger)--move-trigger-from-react-utilities branch 2 times, most recently from 05a3f55 to 730af74 Compare September 23, 2022 14:01
@bsunderhus
Copy link
Contributor Author

Blocked by #24927

@bsunderhus bsunderhus added Status: Blocked Resolution blocked by another issue and removed Status: Blocked Resolution blocked by another issue labels Sep 23, 2022
@bsunderhus bsunderhus force-pushed the chore(react-trigger)--move-trigger-from-react-utilities branch from a779196 to f2c6986 Compare September 26, 2022 08:01
@bsunderhus bsunderhus marked this pull request as ready for review September 26, 2022 08:36
@bsunderhus bsunderhus requested review from a team, behowell and khmakoto as code owners September 26, 2022 08:36
@bsunderhus bsunderhus closed this Sep 26, 2022
@bsunderhus bsunderhus reopened this Sep 26, 2022
ling1726
ling1726 previously approved these changes Sep 26, 2022
@ling1726 ling1726 dismissed their stale review September 26, 2022 13:32

alpha package

@bsunderhus bsunderhus requested a review from ling1726 September 26, 2022 18:08
@bsunderhus bsunderhus force-pushed the chore(react-trigger)--move-trigger-from-react-utilities branch from 6968bef to 60ee223 Compare September 27, 2022 07:29
@bsunderhus bsunderhus requested a review from ling1726 September 27, 2022 07:29
Copy link
Contributor

@behowell behowell left a comment

Choose a reason for hiding this comment

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

This change looks good technically. However, the description doesn't say why it's necessary to move this into a separate package. I'm worried about excessive proliferation of packages if we start moving each utility to its own package. Could you update the description with more details about what problem this is solving?

If you'd like, you can set this PR to auto-merge after updating the description, so it will merge once I see the update and approve.

Thanks!

@bsunderhus bsunderhus requested a review from behowell September 30, 2022 08:51
@bsunderhus bsunderhus enabled auto-merge (squash) September 30, 2022 08:51
Copy link
Contributor

@behowell behowell left a comment

Choose a reason for hiding this comment

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

So it sounds like this is refactoring; not solving a technical problem, right? Before making a new package for this, it would be good to have a plan for how we decide when a utility should live in react-utilities vs. its own package. That way we can have a consistent and predictable set of packages.

If the goal is to keep related utilities together, then perhaps they should just be in a sub-folder of react-utilities, instead of their own package.

Or, if the goal is to separate out the fluentui-isms like triggers and slots, out from general-purpose utilities, then perhaps we name this package react-core or something, and move the slots and other core functionality into it.

Would it be possible to write a (short?) RFC about how that would work, and discuss it with the team at the next tech sync?

@bsunderhus
Copy link
Contributor Author

bsunderhus commented Oct 3, 2022

So it sounds like this is refactoring; not solving a technical problem, right?

Yes, this is definitely just a refactoring, and a simple one.

If the goal is to keep related utilities together, then perhaps they should just be in a sub-folder of react-utilities, instead of their own package.

The problem is not just splitting from general-purpose utilities methods, its more about the priority those domain specific utilities require in our own library.

The way I see utilities provided on hooks folder can be easily documented with comments, as they are independent and punctual.

On the case of both slots and triggers utilities on the other hand, they have their own domain and its very specific use cases (including multiple types and helper methods that require to be used together in a certain pattern). Demanding proper documentation and examples on our side to ensure proper usage, simply adding comments doesn't get this done.

Or, if the goal is to separate out the fluentui-isms like triggers and slots, out from general-purpose utilities, then perhaps we name this package react-core or something, and move the slots and other core functionality into it.

I believe a react-core package would end up being equivalent to our current approach of dropping everything related in the same folder. By splitting those domain specific cases we're giving them the actual relevance they deserve to ensure we'll have them properly documented (which is not necessarily the case at the moment), as an undocumented package is way more critical than an undocumented folder.

I don't see why having a package for each of those complete distinct logics is so harmful, they're treating complete different domain logics and have absolutely nothing in common but the fact that they're reused internally. if it were the case of having those two domains colide by depending on one another than I'd agree with keeping them together, but that is not the case.

@bsunderhus
Copy link
Contributor Author

After internal discussion, I think it's a majority decision to keep the current folder structure the way it is, on the addendum of a commitment on our side for better documentation on those domain specific folders.

@bsunderhus bsunderhus closed this Oct 3, 2022
auto-merge was automatically disabled October 3, 2022 09:49

Pull request was closed

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.

5 participants