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 React as external config #15773

Merged
merged 1 commit into from
Jan 18, 2021
Merged

Fix React as external config #15773

merged 1 commit into from
Jan 18, 2021

Conversation

filoxo
Copy link
Contributor

@filoxo filoxo commented Oct 29, 2020

Pull request checklist

Description of changes

As mentioned in the #10349, fluentui-react's build is configured to be able to use React as an external but only as a global. This change fixes that by using the proper config that will allow using external React as a global as well as with other methodologies (such as with SystemJS for in-browser module resolution).

Focus areas to test

  • Inspect the output bundle and verify that:
    • require("React"),require("ReactDOM") is no longer present
    • the correct casing require("react"),require("react-dom") is included
  • Verify that it still works with React as a global (eg. react provided via a CDN)

Notes

I wasn't able to find how to actually build the final bundle to be able to test these changes as I've described above.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 29, 2020

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 56bb666:

Sandbox Source
Fluent UI Button Configuration
codesandbox-react-template Configuration
codesandbox-react-northstar-template Configuration

@msft-github-bot
Copy link
Contributor

msft-github-bot commented Oct 29, 2020

Perf Analysis

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 830 833 5000
BaseButtonCompat mount 954 900 5000
Breadcrumb mount 43259 43591 5000
Checkbox mount 1528 1517 5000
CheckboxBase mount 1283 1231 5000
ChoiceGroup mount 4736 4799 5000
ComboBox mount 990 1035 1000
CommandBar mount 10297 10360 1000
ContextualMenu mount 6104 6091 1000
DefaultButtonCompat mount 1088 1083 5000
DetailsRow mount 3705 3671 5000
DetailsRowFast mount 3680 3669 5000
DetailsRowNoStyles mount 3496 3461 5000
Dialog mount 1458 1499 1000
DocumentCardTitle mount 1859 1821 1000
Dropdown mount 3287 3320 5000
FocusTrapZone mount 1871 1838 5000
FocusZone mount 1853 1883 5000
IconButtonCompat mount 1760 1744 5000
Label mount 339 340 5000
Layer mount 1766 1771 5000
Link mount 458 465 5000
MakeStyles mount 1982 1959 50000
MenuButtonCompat mount 1490 1434 5000
MessageBar mount 2069 2010 5000
Nav mount 3236 3294 1000
OverflowSet mount 1035 997 5000
Panel mount 1453 1404 1000
Persona mount 862 843 1000
Pivot mount 1401 1393 1000
PrimaryButtonCompat mount 1274 1293 5000
Rating mount 7500 7508 5000
SearchBox mount 1331 1309 5000
Shimmer mount 2535 2596 5000
Slider mount 1899 1932 5000
SpinButton mount 5346 4995 5000
Spinner mount 445 419 5000
SplitButtonCompat mount 3154 3170 5000
Stack mount 524 522 5000
StackWithIntrinsicChildren mount 1532 1569 5000
StackWithTextChildren mount 4492 4429 5000
SwatchColorPicker mount 10220 10388 5000
Tabs mount 1426 1398 1000
TagPicker mount 2934 2823 5000
TeachingBubble mount 11824 11693 5000
Text mount 428 440 5000
TextField mount 1370 1409 5000
ThemeProvider mount 2171 2179 5000
ThemeProvider virtual-rerender 683 652 5000
Toggle mount 810 793 5000
button mount 682 670 5000
buttonNative mount 107 107 5000

Perf Analysis (Fluent)

Perf comparison
Status Scenario Fluent TPI Fabric TPI Ratio Iterations Ticks
🦄 Avatar.Fluent 0.17 0.52 0.33:1 2000 348
🦄 Button.Fluent 0.12 0.21 0.57:1 5000 596
🔧 Checkbox.Fluent 0.67 0.33 2.03:1 1000 665
🎯 Dialog.Fluent 0.17 0.22 0.77:1 5000 835
🔧 Dropdown.Fluent 3.02 0.43 7.02:1 1000 3020
🔧 Icon.Fluent 0.14 0.06 2.33:1 5000 692
🦄 Image.Fluent 0.08 0.13 0.62:1 5000 386
🔧 Slider.Fluent 1.63 0.46 3.54:1 1000 1632
🔧 Text.Fluent 0.07 0.03 2.33:1 5000 374
🦄 Tooltip.Fluent 0.11 0.9 0.12:1 5000 561

🔧 Needs work     🎯 On target     🦄 Amazing

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
ButtonMinimalPerf.default 200 177 1.13:1
ListWith60ListItems.default 679 612 1.11:1
RefMinimalPerf.default 260 235 1.11:1
ChatWithPopoverPerf.default 457 429 1.07:1
ListMinimalPerf.default 534 498 1.07:1
TextMinimalPerf.default 388 363 1.07:1
Icon.Fluent 692 648 1.07:1
ProviderMinimalPerf.default 1008 949 1.06:1
BoxMinimalPerf.default 392 374 1.05:1
StatusMinimalPerf.default 759 722 1.05:1
CustomToolbarPrototype.default 3933 3744 1.05:1
AnimationMinimalPerf.default 432 414 1.04:1
AvatarMinimalPerf.default 216 208 1.04:1
ButtonUseCssNestingPerf.default 1130 1091 1.04:1
LoaderMinimalPerf.default 769 738 1.04:1
MenuButtonMinimalPerf.default 1622 1566 1.04:1
RadioGroupMinimalPerf.default 484 464 1.04:1
SplitButtonMinimalPerf.default 3878 3735 1.04:1
ImageMinimalPerf.default 393 380 1.03:1
ListNestedPerf.default 592 572 1.03:1
TooltipMinimalPerf.default 862 838 1.03:1
TreeWith60ListItems.default 185 180 1.03:1
Avatar.Fluent 348 339 1.03:1
Dialog.Fluent 835 810 1.03:1
AttachmentSlotsPerf.default 1232 1213 1.02:1
ChatMinimalPerf.default 642 631 1.02:1
CheckboxMinimalPerf.default 2927 2865 1.02:1
DropdownMinimalPerf.default 3044 2999 1.02:1
FormMinimalPerf.default 427 418 1.02:1
HeaderMinimalPerf.default 380 374 1.02:1
LayoutMinimalPerf.default 433 423 1.02:1
PortalMinimalPerf.default 175 172 1.02:1
SkeletonMinimalPerf.default 400 394 1.02:1
Slider.Fluent 1632 1594 1.02:1
AttachmentMinimalPerf.default 169 168 1.01:1
ButtonUseCssPerf.default 846 836 1.01:1
CarouselMinimalPerf.default 507 503 1.01:1
PopupMinimalPerf.default 721 713 1.01:1
TableManyItemsPerf.default 2069 2047 1.01:1
Button.Fluent 596 593 1.01:1
Checkbox.Fluent 665 656 1.01:1
CardMinimalPerf.default 578 577 1:1
ChatDuplicateMessagesPerf.default 369 369 1:1
DividerMinimalPerf.default 387 388 1:1
DropdownManyItemsPerf.default 734 733 1:1
FlexMinimalPerf.default 314 315 1:1
InputMinimalPerf.default 1326 1331 1:1
LabelMinimalPerf.default 433 433 1:1
MenuMinimalPerf.default 893 896 1:1
VideoMinimalPerf.default 632 631 1:1
AlertMinimalPerf.default 301 303 0.99:1
ButtonSlotsPerf.default 602 611 0.99:1
DatepickerMinimalPerf.default 47578 47936 0.99:1
GridMinimalPerf.default 359 361 0.99:1
ProviderMergeThemesPerf.default 1673 1688 0.99:1
ReactionMinimalPerf.default 414 417 0.99:1
TableMinimalPerf.default 423 429 0.99:1
TextAreaMinimalPerf.default 492 498 0.99:1
Dropdown.Fluent 3020 3042 0.99:1
Tooltip.Fluent 561 568 0.99:1
ButtonOverridesMissPerf.default 1706 1733 0.98:1
EmbedMinimalPerf.default 4143 4208 0.98:1
HeaderSlotsPerf.default 801 814 0.98:1
Image.Fluent 386 395 0.98:1
Text.Fluent 374 382 0.98:1
ItemLayoutMinimalPerf.default 1243 1283 0.97:1
ListCommonPerf.default 675 699 0.97:1
DialogMinimalPerf.default 809 843 0.96:1
SliderMinimalPerf.default 1601 1665 0.96:1
IconMinimalPerf.default 658 691 0.95:1
ToolbarMinimalPerf.default 946 1001 0.95:1
TreeMinimalPerf.default 788 829 0.95:1
RosterPerf.default 1183 1263 0.94:1
SegmentMinimalPerf.default 369 391 0.94:1
AccordionMinimalPerf.default 154 172 0.9:1

@size-auditor
Copy link

size-auditor bot commented Oct 29, 2020

Asset size changes

⚠️ Insufficient baseline data to detect size changes

Unable to find bundle size details for Baseline commit: 0229138

Possible causes

  • The baseline build 0229138 is broken
  • The Size Auditor run for the baseline build 0229138 was not triggered

Recommendations

  • Please merge your branch for this Pull request with the latest master build and commit your changes once again

@ecraig12345
Copy link
Member

Thanks for doing this! We'll probably want to do it in the other packages as well--if you're interested I can explain how to do that now, or I can just handle it later.

Looks like the build is failing because you're missing a change file--you'll need to run yarn change from the root of the repo then push again. (The other failure is a known intermittent issue.)

@filoxo
Copy link
Contributor Author

filoxo commented Nov 4, 2020

I think I'd be open to fixing the other package's configs as well. Is there any info/docs on how I can build the final assets that are published? Or should I look in the build pipelines somewhere? That way I can follow the 'steps to verify' in the PR description.

I did include a change file but it was generated before the major refactor that happened recently. Should I remove that and regenerate it instead?

@layershifter
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@layershifter
Copy link
Member

@filoxo can you please merge master and recreate a change file? 🐱

@filoxo
Copy link
Contributor Author

filoxo commented Jan 15, 2021

@layershifter done.

@ecraig12345
Copy link
Member

ecraig12345 commented Jan 15, 2021

Sorry for not following up on this sooner! To ensure this is fixed in all packages, I'd recommend making the change here instead (and revert the change in the packages/react webpack config):

externals: {
react: 'React',
'react-dom': 'ReactDOM',
},

There are a couple other places in that file which also have externals configs, but I don't think those are important to change since they're for specific scenarios where only the global version is needed.

@filoxo
Copy link
Contributor Author

filoxo commented Jan 15, 2021

I tried regenerating the change file but the command returns with No change files are needed. 🤷🏼

@ecraig12345 ecraig12345 merged commit 9ffa0f0 into microsoft:master Jan 18, 2021
ecraig12345 added a commit that referenced this pull request Jan 21, 2021
ecraig12345 added a commit that referenced this pull request Jan 23, 2021
ecraig12345 added a commit to ecraig12345/office-ui-fabric-react that referenced this pull request Jan 23, 2021
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.

office-ui-fabric-react.umd.js requires "ReactDOM"
6 participants