Skip to content
This repository was archived by the owner on Mar 4, 2020. It is now read-only.

fix(useStateManager): keep reference to actions#2347

Merged
layershifter merged 4 commits intomasterfrom
fix/use-state-manager
Feb 12, 2020
Merged

fix(useStateManager): keep reference to actions#2347
layershifter merged 4 commits intomasterfrom
fix/use-state-manager

Conversation

@layershifter
Copy link
Member

@layershifter layershifter commented Feb 12, 2020 •
edited
Loading

This PR fixes an issue with useStateManager() hook to keep the same references for actions as currently it's impossible to use it with ReactuseCallback() hook:

const handleClick = React.useCallback(() => {
  actions.toggle()
}, [actions])

@layershifter layershifter added 🚀 ready for review 🧰 fix Introduces fix for broken behavior. labels Feb 12, 2020
@DustyTheBot
Copy link
Collaborator

DustyTheBot commented Feb 12, 2020 •
edited
Loading

Perf comparison

Status Scenario Fluent TPI Fabric TPI Ratio Iterations Ticks
🔧 Avatar.Fluent 0.49 0.37 1.32:1 2000 980
🦄 Button.Fluent 0.11 0.17 0.65:1 1000 110
🔧 Checkbox.Fluent 0.77 0.29 2.66:1 1000 768
🔧 Dialog.Fluent 0.33 0.17 1.94:1 5000 1652
🔧 Dropdown.Fluent 3.66 0.44 8.32:1 1000 3657
🔧 Icon.Fluent 0.12 0.03 4:1 5000 596
🎯 Image.Fluent 0.05 0.07 0.71:1 5000 252
🔧 Slider.Fluent 1.42 0.3 4.73:1 1000 1415
🔧 Text.Fluent 0.05 0.02 2.5:1 5000 274
🦄 Tooltip.Fluent 0.18 18.94 0.01:1 5000 919

🔧 Needs work     🎯 On target     🦄 Amazing

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
RefMinimalPerf.default 200 177 1.13:1
SliderMinimalPerf.default 2006 1823 1.1:1
TextMinimalPerf.default 278 253 1.1:1
Image.Fluent 252 230 1.1:1
ChatMinimalPerf.default 1905 1744 1.09:1
LabelMinimalPerf.default 942 866 1.09:1
RadioGroupMinimalPerf.default 484 447 1.08:1
ToolbarMinimalPerf.default 769 714 1.08:1
DropdownMinimalPerf.default 3728 3478 1.07:1
SplitButtonMinimalPerf.default 13422 12490 1.07:1
Text.Fluent 274 257 1.07:1
VideoMinimalPerf.default 710 671 1.06:1
AvatarMinimalPerf.default 566 538 1.05:1
PortalMinimalPerf.default 228 218 1.05:1
TooltipMinimalPerf.default 1215 1154 1.05:1
Avatar.Fluent 980 931 1.05:1
AnimationMinimalPerf.default 468 449 1.04:1
AlertMinimalPerf.default 599 582 1.03:1
BoxMinimalPerf.default 246 239 1.03:1
ChatDuplicateMessagesPerf.default 658 638 1.03:1
HeaderSlotsPerf.default 1343 1306 1.03:1
ImageMinimalPerf.default 217 210 1.03:1
MenuButtonMinimalPerf.default 1459 1412 1.03:1
Icon.Fluent 596 576 1.03:1
GridMinimalPerf.default 852 836 1.02:1
HeaderMinimalPerf.default 461 452 1.02:1
ListMinimalPerf.default 311 305 1.02:1
SegmentMinimalPerf.default 1740 1711 1.02:1
StatusMinimalPerf.default 242 238 1.02:1
TableMinimalPerf.default 590 581 1.02:1
CustomToolbarPrototype.default 3873 3831 1.01:1
TreeMinimalPerf.default 854 845 1.01:1
Dropdown.Fluent 3657 3606 1.01:1
ButtonMinimalPerf.default 131 131 1:1
EmbedMinimalPerf.default 6425 6451 1:1
IconMinimalPerf.default 274 274 1:1
PopupMinimalPerf.default 328 328 1:1
ProviderMinimalPerf.default 595 594 1:1
CarouselMinimalPerf.default 1990 2019 0.99:1
DialogMinimalPerf.default 1677 1698 0.99:1
ReactionMinimalPerf.default 3285 3334 0.99:1
AccordionMinimalPerf.default 190 193 0.98:1
AttachmentMinimalPerf.default 1009 1026 0.98:1
AttachmentSlotsPerf.default 3466 3539 0.98:1
CheckboxMinimalPerf.default 3978 4044 0.98:1
LayoutMinimalPerf.default 552 563 0.98:1
LoaderMinimalPerf.default 2455 2515 0.98:1
Checkbox.Fluent 768 780 0.98:1
ChatWithPopoverPerf.default 638 657 0.97:1
FlexMinimalPerf.default 350 361 0.97:1
FormMinimalPerf.default 734 756 0.97:1
InputMinimalPerf.default 902 927 0.97:1
TextAreaMinimalPerf.default 2940 3016 0.97:1
Dialog.Fluent 1652 1708 0.97:1
ButtonSlotsPerf.default 669 695 0.96:1
Slider.Fluent 1415 1476 0.96:1
DropdownManyItemsPerf.default 426 447 0.95:1
DividerMinimalPerf.default 916 971 0.94:1
MenuMinimalPerf.default 1869 1979 0.94:1
Button.Fluent 110 117 0.94:1
HierarchicalTreeMinimalPerf.default 806 863 0.93:1
Tooltip.Fluent 919 994 0.92:1
ItemLayoutMinimalPerf.default 1756 1962 0.9:1
ListCommonPerf.default 816 907 0.9:1
ProviderMergeThemesPerf.default 1078 1202 0.9:1

Generated by 🚫 dangerJS

miroslavstastny
miroslavstastny approved these changes Feb 12, 2020
View reviewed changes
packages/react-bindings/src/hooks/useStateManager.ts Outdated

if (process.env.NODE_ENV === 'production') {
return { state: latestManager.current.state, actions: latestManager.current.actions }
return { state: latestManager.current.state, actions: latestActions }
Copy link
Member

@miroslavstastny miroslavstastny Feb 12, 2020

Choose a reason for hiding this comment

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

As the two return statements now differ only in state I would rather change them to single return with ternary inside

@layershifter layershifter merged commit 25f7bc2 into master Feb 12, 2020
@layershifter layershifter deleted the fix/use-state-manager branch February 12, 2020 16:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@dvdzkwsk dvdzkwsk Awaiting requested review from dvdzkwsk

@dzearing dzearing Awaiting requested review from dzearing

@ecraig12345 ecraig12345 Awaiting requested review from ecraig12345 ecraig12345 is a code owner

@JasonGore JasonGore Awaiting requested review from JasonGore JasonGore is a code owner

@jdhuntington jdhuntington Awaiting requested review from jdhuntington

@kenotron kenotron Awaiting requested review from kenotron

@levithomason levithomason Awaiting requested review from levithomason levithomason is a code owner

@mnajdova mnajdova Awaiting requested review from mnajdova

1 more reviewer

@miroslavstastny miroslavstastny miroslavstastny approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

🧰 fix Introduces fix for broken behavior. 🚀 ready for review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@layershifter @DustyTheBot @miroslavstastny