-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Remove commons export at component root #21660
Remove commons export at component root #21660
Conversation
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 939639c:
|
📊 Bundle size reportUnchanged fixtures
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: b2b3ec1da3d75ad5fc54654645bb992340679d29 (build) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with a bunch of very picky nits.
change/@fluentui-react-accordion-43c26446-45ae-4064-a65f-ceec8761d78e.json
Outdated
Show resolved
Hide resolved
Perf Analysis (
|
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
Avatar | mount | 860 | 826 | 5000 | |
BaseButton | mount | 968 | 989 | 5000 | |
Breadcrumb | mount | 2781 | 2761 | 1000 | |
ButtonNext | mount | 498 | 509 | 5000 | |
Checkbox | mount | 1611 | 1588 | 5000 | |
CheckboxBase | mount | 1366 | 1355 | 5000 | |
ChoiceGroup | mount | 4945 | 4950 | 5000 | |
ComboBox | mount | 993 | 1017 | 1000 | |
CommandBar | mount | 10696 | 10569 | 1000 | |
ContextualMenu | mount | 8741 | 8583 | 1000 | |
DefaultButton | mount | 1200 | 1214 | 5000 | |
DetailsRow | mount | 3819 | 3890 | 5000 | |
DetailsRowFast | mount | 3878 | 3891 | 5000 | |
DetailsRowNoStyles | mount | 3703 | 3735 | 5000 | |
Dialog | mount | 3097 | 3113 | 1000 | |
DocumentCardTitle | mount | 191 | 183 | 1000 | |
Dropdown | mount | 3337 | 3320 | 5000 | |
FluentProviderNext | mount | 1959 | 1943 | 5000 | |
FluentProviderWithTheme | mount | 170 | 173 | 10 | |
FluentProviderWithTheme | virtual-rerender | 109 | 136 | 10 | |
FluentProviderWithTheme | virtual-rerender-with-unmount | 195 | 198 | 10 | |
FocusTrapZone | mount | 1910 | 1892 | 5000 | |
FocusZone | mount | 1821 | 1877 | 5000 | |
IconButton | mount | 1793 | 1833 | 5000 | |
Label | mount | 392 | 378 | 5000 | |
Layer | mount | 3134 | 3139 | 5000 | |
Link | mount | 513 | 519 | 5000 | |
MakeStyles | mount | 1783 | 1732 | 50000 | |
MenuButton | mount | 1551 | 1576 | 5000 | |
MessageBar | mount | 2104 | 2064 | 5000 | |
Nav | mount | 3377 | 3370 | 1000 | |
OverflowSet | mount | 1159 | 1135 | 5000 | |
Panel | mount | 2575 | 2600 | 1000 | |
Persona | mount | 888 | 895 | 1000 | |
Pivot | mount | 1490 | 1508 | 1000 | |
PrimaryButton | mount | 1362 | 1341 | 5000 | |
Rating | mount | 7868 | 7949 | 5000 | |
SearchBox | mount | 1401 | 1372 | 5000 | |
Shimmer | mount | 2618 | 2613 | 5000 | |
Slider | mount | 2049 | 2013 | 5000 | |
SpinButton | mount | 5162 | 5118 | 5000 | |
Spinner | mount | 473 | 466 | 5000 | |
SplitButton | mount | 3237 | 3298 | 5000 | |
Stack | mount | 555 | 567 | 5000 | |
StackWithIntrinsicChildren | mount | 2370 | 2338 | 5000 | |
StackWithTextChildren | mount | 5395 | 5390 | 5000 | |
SwatchColorPicker | mount | 12603 | 11794 | 5000 | |
TagPicker | mount | 2741 | 2759 | 5000 | |
TeachingBubble | mount | 13461 | 13544 | 5000 | |
Text | mount | 457 | 449 | 5000 | |
TextField | mount | 1491 | 1449 | 5000 | |
ThemeProvider | mount | 1319 | 1275 | 5000 | |
ThemeProvider | virtual-rerender | 650 | 672 | 5000 | |
ThemeProvider | virtual-rerender-with-unmount | 1988 | 1951 | 5000 | |
Toggle | mount | 842 | 838 | 5000 | |
buttonNative | mount | 159 | 157 | 5000 |
Perf Analysis (@fluentui/react-northstar
)
@behowell considering that api extractor doesn't actually benefit from having commons exported from the types file, I'd certainly consider exporting it on a per case basis. |
scaling this back to only change the index file for components that need to share the commons type internally. All others will just have the export removed |
b011008
to
457b332
Compare
Current Behavior
Commons types are exported from the root of each component, and could be taken on as a dependency if you imported directly from the component. We want to eventually remove the common types, and don't want anyone taking dependencies on them.
New Behavior
This PR removes the exporting of most Commons types, and changes the template for future components. For those that require sharing Commons between components, the Commons type is omitted from the top level export.
Related Issue(s)
This is a replacement for #21607
Fixes #