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.

Sign up for GitHub

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

Jump to bottom

fix: update implementation of getReactFiberFromNode to be compatible with React 17 #16392

Merged
merged 4 commits into from
Jan 6, 2021
Merged

fix: update implementation of getReactFiberFromNode to be compatible with React 17 #16392

merged 4 commits into from
Jan 6, 2021

Conversation

layershifter
Copy link
Member

Fixes #16192.

In React 17 internal keys were changed (facebook/react#18377), I performed a small refactor to extract shared code (Popper and FiberNavigator) to a single place and added unit tests. Also manually tested with React 17.

@msft-fluent-ui-bot msft-fluent-ui-bot added the Fluent UI react-northstar (v0) Work related to Fluent UI V0 label Jan 6, 2021
@layershifter layershifter changed the title fix: update implementaion of getReactFiberFromNode to be compatible with React 17 fix: update implementation of getReactFiberFromNode to be compatible with React 17 Jan 6, 2021
@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 6, 2021 •
edited
Loading

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 020a5e1:

Sandbox Source
Fluent UI Button Configuration
codesandbox-react-template Configuration
codesandbox-react-northstar-template Configuration
kind-dewdney-2lj99 Issue #16192

@fabricteam
Copy link
Collaborator

fabricteam commented Jan 6, 2021 •
edited
Loading

Perf Analysis

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 875 895 5000
BaseButtonCompat mount 997 970 5000
Breadcrumb mount 44752 44601 5000
Checkbox mount 1616 1683 5000
CheckboxBase mount 1362 1368 5000
ChoiceGroup mount 5080 5201 5000
ComboBox mount 965 1038 1000
CommandBar mount 10583 10611 1000
ContextualMenu mount 6470 6547 1000
DefaultButtonCompat mount 1248 1243 5000
DetailsRow mount 4056 4091 5000
DetailsRowFast mount 4016 4060 5000
DetailsRowNoStyles mount 3778 3760 5000
Dialog mount 1592 1581 1000
DocumentCardTitle mount 1864 1871 1000
Dropdown mount 3621 3571 5000
FocusTrapZone mount 1893 1914 5000
FocusZone mount 1943 1872 5000
IconButtonCompat mount 1962 1951 5000
Label mount 356 359 5000
Layer mount 1951 1925 5000
Link mount 505 506 5000
MakeStyles mount 2073 2046 50000
MenuButtonCompat mount 1590 1594 5000
MessageBar mount 2115 2067 5000
Nav mount 3499 3560 1000
OverflowSet mount 1092 1089 5000
Panel mount 1557 1510 1000
Persona mount 892 916 1000
Pivot mount 1473 1497 1000
PrimaryButtonCompat mount 1400 1387 5000
Rating mount 8145 8307 5000
SearchBox mount 1456 1457 5000
Shimmer mount 2816 2881 5000
Slider mount 2022 2071 5000
SpinButton mount 5360 5329 5000
Spinner mount 460 461 5000
SplitButtonCompat mount 3463 3424 5000
Stack mount 535 558 5000
StackWithIntrinsicChildren mount 1661 1613 5000
StackWithTextChildren mount 4982 5096 5000
SwatchColorPicker mount 10928 10825 5000
Tabs mount 1532 1496 1000
TagPicker mount 3093 3028 5000
TeachingBubble mount 12162 12212 5000
Text mount 451 451 5000
TextField mount 1490 1501 5000
ThemeProvider mount 2218 2232 5000
ThemeProvider virtual-rerender 676 692 5000
Toggle mount 882 880 5000
button mount 726 730 5000
buttonNative mount 115 114 5000

Perf Analysis (Fluent)

Perf comparison
Status Scenario Fluent TPI Fabric TPI Ratio Iterations Ticks
🎯 Avatar.Fluent 0.49 0.56 0.87:1 2000 989
🦄 Button.Fluent 0.13 0.22 0.59:1 5000 657
🔧 Checkbox.Fluent 0.7 0.39 1.79:1 1000 700
🎯 Dialog.Fluent 0.18 0.24 0.75:1 5000 897
🔧 Dropdown.Fluent 3.21 0.45 7.13:1 1000 3209
🔧 Icon.Fluent 0.16 0.07 2.29:1 5000 807
🦄 Image.Fluent 0.09 0.13 0.69:1 5000 470
🔧 Slider.Fluent 1.7 0.47 3.62:1 1000 1696
🔧 Text.Fluent 0.09 0.04 2.25:1 5000 441
🦄 Tooltip.Fluent 0.12 0.94 0.13:1 5000 594

🔧 Needs work     🎯 On target     🦄 Amazing

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
TextAreaMinimalPerf.default 620 564 1.1:1
FormMinimalPerf.default 530 486 1.09:1
HeaderMinimalPerf.default 464 427 1.09:1
GridMinimalPerf.default 424 391 1.08:1
Icon.Fluent 807 747 1.08:1
MenuButtonMinimalPerf.default 1827 1713 1.07:1
Checkbox.Fluent 700 655 1.07:1
LayoutMinimalPerf.default 485 456 1.06:1
MenuMinimalPerf.default 1008 954 1.06:1
TableManyItemsPerf.default 2444 2305 1.06:1
Image.Fluent 470 443 1.06:1
CarouselMinimalPerf.default 511 486 1.05:1
TreeMinimalPerf.default 935 892 1.05:1
Avatar.Fluent 989 942 1.05:1
AccordionMinimalPerf.default 187 180 1.04:1
AlertMinimalPerf.default 341 328 1.04:1
AvatarMinimalPerf.default 527 508 1.04:1
ButtonMinimalPerf.default 216 208 1.04:1
CardMinimalPerf.default 673 650 1.04:1
DividerMinimalPerf.default 460 442 1.04:1
RadioGroupMinimalPerf.default 516 498 1.04:1
TableMinimalPerf.default 496 475 1.04:1
ToolbarMinimalPerf.default 1053 1016 1.04:1
ButtonSlotsPerf.default 673 651 1.03:1
DropdownMinimalPerf.default 3196 3099 1.03:1
PortalMinimalPerf.default 175 170 1.03:1
Dropdown.Fluent 3209 3117 1.03:1
Slider.Fluent 1696 1642 1.03:1
AttachmentMinimalPerf.default 200 197 1.02:1
LabelMinimalPerf.default 488 479 1.02:1
ListWith60ListItems.default 1029 1010 1.02:1
RefMinimalPerf.default 247 242 1.02:1
StatusMinimalPerf.default 847 832 1.02:1
VideoMinimalPerf.default 744 731 1.02:1
Text.Fluent 441 431 1.02:1
ButtonOverridesMissPerf.default 1854 1832 1.01:1
ChatMinimalPerf.default 709 699 1.01:1
ChatWithPopoverPerf.default 521 514 1.01:1
ImageMinimalPerf.default 464 460 1.01:1
ListCommonPerf.default 726 716 1.01:1
LoaderMinimalPerf.default 820 812 1.01:1
ProviderMinimalPerf.default 1017 1007 1.01:1
SkeletonMinimalPerf.default 445 441 1.01:1
SliderMinimalPerf.default 1672 1661 1.01:1
Dialog.Fluent 897 885 1.01:1
ButtonUseCssNestingPerf.default 1192 1187 1:1
DatepickerMinimalPerf.default 51826 51714 1:1
FlexMinimalPerf.default 343 342 1:1
InputMinimalPerf.default 1416 1412 1:1
ItemLayoutMinimalPerf.default 1391 1397 1:1
ListNestedPerf.default 661 660 1:1
PopupMinimalPerf.default 762 765 1:1
Button.Fluent 657 657 1:1
AttachmentSlotsPerf.default 1326 1344 0.99:1
ButtonUseCssPerf.default 895 906 0.99:1
CheckboxMinimalPerf.default 3015 3033 0.99:1
DropdownManyItemsPerf.default 833 845 0.99:1
EmbedMinimalPerf.default 4423 4473 0.99:1
HeaderSlotsPerf.default 911 919 0.99:1
ProviderMergeThemesPerf.default 1640 1663 0.99:1
SplitButtonMinimalPerf.default 4195 4228 0.99:1
CustomToolbarPrototype.default 3902 3938 0.99:1
Tooltip.Fluent 594 602 0.99:1
BoxMinimalPerf.default 426 433 0.98:1
ChatDuplicateMessagesPerf.default 456 463 0.98:1
TextMinimalPerf.default 408 415 0.98:1
TreeWith60ListItems.default 223 228 0.98:1
AnimationMinimalPerf.default 447 460 0.97:1
ListMinimalPerf.default 558 576 0.97:1
ReactionMinimalPerf.default 480 496 0.97:1
SegmentMinimalPerf.default 429 441 0.97:1
DialogMinimalPerf.default 858 898 0.96:1
TooltipMinimalPerf.default 913 955 0.96:1
IconMinimalPerf.default 760 809 0.94:1

@size-auditor
Copy link

size-auditor bot commented Jan 6, 2021 •
edited
Loading

Asset size changes

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

Baseline commit: 5feec8dc74dd5db94341c69ca9254e8af210488e (build)

ling1726
ling1726 reviewed Jan 6, 2021
View reviewed changes
packages/fluentui/react-northstar/src/utils/getReactFiberFromNode.ts
}
}

throw new Error('getReactFiber(): Failed to find a React Fiber on a node');
Copy link
Member

Choose a reason for hiding this comment

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

nit: consider having a test case for the throw

Copy link
Member Author

Choose a reason for hiding this comment

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

Added 👍

@layershifter
add a UT
6f35a75
@layershifter
Merge branch 'master' of https://github.com/microsoft/fluentui into f…
020a5e1 
…ix/popper-warning

� Conflicts:
�	packages/fluentui/CHANGELOG.md
@layershifter layershifter merged commit 856a4e5 into master Jan 6, 2021
@layershifter layershifter deleted the fix/popper-warning branch January 6, 2021 17:32
@sushruth
Copy link

sushruth commented Jan 6, 2021 •
edited
Loading

Which version of @fluentui/react-northstar will have this fix? Currently this is affecting unit tests on apps using @fluentui/react-northstar and react 17.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ling1726 ling1726 ling1726 approved these changes

@assuncaocharles assuncaocharles Awaiting requested review from assuncaocharles

@dzearing dzearing Awaiting requested review from dzearing

@jurokapsiar jurokapsiar Awaiting requested review from jurokapsiar

@miroslavstastny miroslavstastny Awaiting requested review from miroslavstastny

@pompomon pompomon Awaiting requested review from pompomon

Assignees

@layershifter layershifter

Labels
Fluent UI react-northstar (v0) Work related to Fluent UI V0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Popper: throws an error in dev check
5 participants
@layershifter @fabricteam @sushruth @ling1726 @msft-fluent-ui-bot