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

Add missing peers on react/react-dom #23697

Closed
wants to merge 8 commits into from

Conversation

Stuart-Wilcox
Copy link

@Stuart-Wilcox Stuart-Wilcox commented Jun 23, 2022

Current Behavior

Several packages are missing declarations of peer dependencies. Peers are declared on certain @FluentUI packages, then other @FluentUI packages which depend on them do not fulfil the peer requirements by "forwarding" them explicitly through declaring them as their own peers. This causes issues when attempting to use @FluentUI packages in a "strict" dependency environment.

The solution is to declare the peers as needed so they can be satisfied by the end consumer of the package.

New Behavior

Peers have been appropriately declared where needed

Package Name Reason
@fluentui/font-icons-mdl2 declare peers to satisfy @fluentui/style-utilities
@fluentui/react-shared-contexts declare peers to satisfy @fluentui/react-theme
@fluentui/react-icons-mdl2 declare peers to satisfy @fluentui/react-icon-provider
@fluentui/scheme-utilities declare peers to satisfy @fluentui/theme
@fluentui/style-utilities declare peers to satisfy @fluentui/utiltities
@fluentui/theme-samples declare peers to satisfy @fluentui/react

Related Issue(s)

#23110
Fixes #

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jun 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 6734c74:

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

@size-auditor
Copy link

size-auditor bot commented Jun 23, 2022

Asset size changes

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

Baseline commit: 460a100b69c8d95cfbb3e33bf3340ec2a408581b (build)

@fabricteam
Copy link
Collaborator

fabricteam commented Jun 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 1381 1387 5000
Button mount 1025 1053 5000
FluentProvider mount 1722 1751 5000
FluentProviderWithTheme mount 739 672 10
FluentProviderWithTheme virtual-rerender 639 674 10
FluentProviderWithTheme virtual-rerender-with-unmount 715 711 10
MakeStyles mount 2118 2105 50000

@fabricteam
Copy link
Collaborator

fabricteam commented Jun 23, 2022

📊 Bundle size report

Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-accordion
Accordion (including children components)
77.836 kB
23.683 kB
react-avatar
Avatar
46.757 kB
13.737 kB
react-avatar
AvatarGroup
136.262 kB
40.676 kB
react-avatar
AvatarGroupItem
66.472 kB
19.079 kB
react-badge
Badge
21.653 kB
6.862 kB
react-badge
CounterBadge
22.557 kB
7.172 kB
react-badge
PresenceBadge
23.357 kB
7.099 kB
react-button
Button
35.704 kB
9.499 kB
react-button
CompoundButton
42.777 kB
10.733 kB
react-button
MenuButton
38.279 kB
10.365 kB
react-button
SplitButton
45.612 kB
11.71 kB
react-button
ToggleButton
50.929 kB
10.919 kB
react-card
Card - All
66.957 kB
19.042 kB
react-card
Card
62.113 kB
17.911 kB
react-card
CardFooter
8.461 kB
3.555 kB
react-card
CardHeader
10.026 kB
4.067 kB
react-card
CardPreview
8.562 kB
3.61 kB
react-combobox
Combobox (including child components)
73.69 kB
23.901 kB
react-combobox
Dropdown (including child components)
73.203 kB
23.895 kB
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
189.067 kB
52.144 kB
react-components
react-components: FluentProvider & webLightTheme
31.883 kB
10.516 kB
react-divider
Divider
16.321 kB
5.837 kB
react-image
Image
10.68 kB
4.215 kB
react-input
Input
22.861 kB
7.512 kB
react-label
Label
9.238 kB
3.815 kB
react-link
Link
12.082 kB
4.88 kB
react-menu
Menu (including children components)
116.688 kB
35.475 kB
react-menu
Menu (including selectable components)
119.863 kB
35.96 kB
react-overflow
hooks only
10.839 kB
4.146 kB
react-popover
Popover
104.924 kB
31.964 kB
react-portal
Portal
10.033 kB
3.753 kB
react-positioning
usePositioning
23.839 kB
8.292 kB
react-provider
FluentProvider
14.76 kB
5.596 kB
react-radio
Radio
30.438 kB
10.41 kB
react-radio
RadioGroup
14.319 kB
5.711 kB
react-select
Select
20.002 kB
7.171 kB
react-slider
Slider
26.136 kB
8.387 kB
react-spinbutton
SpinButton
43.378 kB
12.277 kB
react-spinner
Spinner
19.073 kB
6.308 kB
react-switch
Switch
26.578 kB
8.6 kB
react-text
Text - Default
11.572 kB
4.537 kB
react-text
Text - Wrappers
14.882 kB
4.977 kB
react-textarea
Textarea
22.228 kB
7.523 kB
react-theme
Single theme token import
69 B
89 B
react-theme
Teams: all themes
28.995 kB
6.215 kB
react-theme
Teams: Light theme
16.973 kB
4.86 kB
react-tooltip
Tooltip
44.55 kB
15.386 kB
🤖 This report was generated against 460a100b69c8d95cfbb3e33bf3340ec2a408581b

@fabricteam
Copy link
Collaborator

fabricteam commented Jun 23, 2022

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
BaseButton mount 784 788 5000
Breadcrumb mount 2367 2348 1000
Checkbox mount 1280 1293 5000
CheckboxBase mount 1088 1083 5000
ChoiceGroup mount 4060 4012 5000
ComboBox mount 841 837 1000
CommandBar mount 9130 9105 1000
ContextualMenu mount 10262 10408 1000
DefaultButton mount 977 997 5000
DetailsRow mount 3288 3285 5000
DetailsRowFast mount 3284 3302 5000
DetailsRowNoStyles mount 3122 3158 5000
Dialog mount 2465 2431 1000
DocumentCardTitle mount 153 152 1000
Dropdown mount 2906 2878 5000
FocusTrapZone mount 1641 1596 5000
FocusZone mount 1568 1581 5000
IconButton mount 1511 1500 5000
Label mount 291 298 5000
Layer mount 2527 2532 5000
Link mount 392 407 5000
MenuButton mount 1283 1282 5000
MessageBar mount 1854 1786 5000
Nav mount 2834 2879 1000
OverflowSet mount 948 939 5000
Panel mount 1907 1853 1000
Persona mount 866 853 1000
Pivot mount 1259 1252 1000
PrimaryButton mount 1121 1125 5000
Rating mount 6684 6616 5000
SearchBox mount 1116 1116 5000
Shimmer mount 2104 2114 5000
Slider mount 1675 1661 5000
SpinButton mount 4322 4339 5000
Spinner mount 365 387 5000
SplitButton mount 2769 2733 5000
Stack mount 444 441 5000
StackWithIntrinsicChildren mount 1992 1989 5000
StackWithTextChildren mount 4498 4540 5000
SwatchColorPicker mount 10080 10025 5000
TagPicker mount 2341 2324 5000
TeachingBubble mount 85515 83672 5000
Text mount 369 364 5000
TextField mount 1209 1182 5000
ThemeProvider mount 1026 1020 5000
ThemeProvider virtual-rerender 560 560 5000
ThemeProvider virtual-rerender-with-unmount 1636 1601 5000
Toggle mount 682 676 5000
buttonNative mount 111 114 5000

@Stuart-Wilcox Stuart-Wilcox marked this pull request as ready for review June 23, 2022 17:42
packages/react-focus/package.json Outdated Show resolved Hide resolved
@layershifter
Copy link
Member

This causes issues when attempting to use @FluentUI packages in a "strict" dependency environment.

Do I correctly understand that it's a requirement created by midgard-yarn-strict? I don't think that it's a correct approach for dependency management and I have already raised it in #23639 (comment). I would like to discuss this with @microsoft/fluentui-react-build once members will return from their vacations.


I unblocked this PR, but anyway please address https://github.com/microsoft/fluentui/pull/23697/files#r909309590.

@Stuart-Wilcox Stuart-Wilcox changed the title add a bunch of missing peers on react/react-dom Add missing peers on react/react-dom Jun 30, 2022
@Stuart-Wilcox
Copy link
Author

This causes issues when attempting to use @FluentUI packages in a "strict" dependency environment.

Do I correctly understand that it's a requirement created by midgard-yarn-strict? I don't think that it's a correct approach for dependency management and I have already raised it in #23639 (comment). I would like to discuss this with @microsoft/fluentui-react-build once members will return from their vacations.

I unblocked this PR, but anyway please address https://github.com/microsoft/fluentui/pull/23697/files#r909309590.

You are correct, that this is in relation to midgard-yarn-strict. I have made a couple more updates to the PR, I would love to discuss with whoever needed

@@ -31,6 +31,12 @@
"@fluentui/utilities": "^8.8.3",
"tslib": "^2.1.0"
},
"peerDependencies": {
"@types/react": ">=16.8.0 <18.0.0",
"@types/react-dom": ">=16.8.0 <18.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

@types/* are devDependencies and shouldn't be required to be installed by any downstream dependency. Right?

Copy link
Member

Choose a reason for hiding this comment

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

Also font-icons-mdl2 does not use react. I'm not understanding what's going on here.

Copy link
Member

Choose a reason for hiding this comment

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

It is feasible that font-icons-mdl2 could have react/react-dom as a devDependency if we were defining strict dependencies. This might be required for running tests at this layer in isolation, which would exercise the utilities paths. Peer dependencies would then be "resolved" at this point.

But a change like that should not affect downstream repos. A downstream repo who installs font-icons-mdl2 will still need to resolve the transient peer deps at that layer regardless of the resolution of the intermediate layers which don't directly depend on react.

Copy link
Member

@dzearing dzearing left a comment

Choose a reason for hiding this comment

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

I don't understand the need for this change. If packages now need to be responsible for forwarding dependencies' peer requirements, we are in for a huge mess. Let's examine the need here.

@layershifter
Copy link
Member

I don't understand the need for this change. If packages now need to be responsible for forwarding dependencies' peer requirements, we are in for a huge mess. Let's examine the need here.

FYI: I created an issue in midgard-yarn-strict repository with a minimal repro and needed details: VincentBailly/midgard-yarn-strict#9

@msft-fluent-ui-bot
Copy link
Collaborator

This pull request has been automatically marked as stale because it was marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 5 days of this comment. Thank you for your contributions to Fluent UI!

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.

None yet

6 participants