microsoft / fluentui

Fluent UI web represents a collection of utilities, React components, and web components for building web applications.
https://react.fluentui.dev
Other
18.33k stars 2.72k forks source link

Performance: Why is DetailListBase not cached it styled ? #8764

Closed LiangMingChen closed 4 years ago

LiangMingChen commented 5 years ago

Environment Information

Please provide a reproduction of the bug in a codepen:

Actual behavior:

DetailListBase componentWill receiveProps always get call due to styled changed.

image

node_modules\office-ui-fabric-react\lib\components\DetailsList\DetailsList.styles.js

getStyles is not memoized.

D:\AS\1\Aria\honolulu-client\node_modules\office-ui-fabric-react\lib\components\DetailsList\DetailsList.js

import { styled } from '../../Utilities'; import { DetailsListBase } from './DetailsList.base'; import { getStyles } from './DetailsList.styles'; export var DetailsList = styled(DetailsListBase, getStyles, undefined, { scope: 'DetailsList' });

this styled always create a new proped for DetailListBase.

Expected behavior:

Nothing should change. it should memoized the get Styles

Priorities and help requested:

Are you willing to submit a PR to fix? (Yes, No)

Requested priority: (Blocking, High, Normal, Low)

Products/sites affected: (if applicable)

KevinTCoughlin commented 5 years ago

Thanks for the report @LiangMingChen.

This is definitely worth fixing for performance. Last time I briefly investigated this I couldn't dive far enough into the styles call to see what was churning.

LiangMingChen commented 5 years ago

Thanks for the report @LiangMingChen.

This is definitely worth fixing for performance. Last time I briefly investigated this I couldn't dive far enough into the styles call to see what was churning.

I open a issue to styled tag https://github.com/OfficeDev/office-ui-fabric-react/issues/8769

cliffkoh commented 5 years ago

@dzearing

dzearing commented 5 years ago

I don't believe we should be memoizing at the .styles.ts layer, but rather in the getClassNames helper, which there is a PR out for.

xugao commented 4 years ago

looks like this was fixed, by #8761. @LiangMingChen - feel free to double check