-
Notifications
You must be signed in to change notification settings - Fork 53
[WIP] feat(create-component): add create component #2200
Conversation
const stylesAdditions: any = {}; | ||
const variantNames: string[] = []; | ||
|
||
const componentStyles = |
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.
Since TypeScript 3.7 introduced optional chaining you can use it here:
const componentStyles = theme?.components?.[componentName]?.styles ? ... : ...
Ref. https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html
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.
Same below for variants
withDebugId({ root: props.design }, 'props.design'), | ||
withDebugId({ root: props.styles }, 'props.styles'), | ||
withDebugId({ root: animationCSSProp }, 'props.animation'), | ||
// TODO: update mergeComponentStyles to cache its results based on theme object and prop combinations. |
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.
@levithomason @jdhuntington regarding this comment, I made updates (added mergeComponentStylesWithCache
) for caching.. This required to do some changes in the factories.ts (in the way of how the defaultProps, userProps and overrideProps' styles are merged (I am considering them all to be flat object for now - I know this is not ideal and may need some rethinking, but anyway, just sharing the current state. This was needed for the hashing to work...)
Let me know what you think..
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.
Update - there are some perf gains with this:
Example with 1500 Menus, with different combinations of props, that repeatedly appear:
master avg: 5370.17
current avg: 4541.11
Chat with popover
master avg: 626.84
current avg: 596.07
ChatDuplicateMessages
master avg: 557.09
current avg: 398.57
I had to add hash (id) on the theme - if it is not specified I am generating it as the JSON.stringify of the theme object (this is costly, is that's why for now I added hash on the different themes, but we can review it later). If it is useful I can move these changes to a separate PR and iterate on that.
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.
Hm, I would expect this to be much faster. Are we sure it is working?
I added some comments on the cached function, have a look.
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.
Also, it would be great to see the results of yarn perf
on master as a baseline in this PR and another table with the current commit.
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.
Here is a diff:
| Example | MASTER | CURRENT
| | Avg | Avg
| -------------------------------- | ------ | ------
| ProviderMergeThemes.perf.tsx | 751.44 | 675.26
| DropdownManyItems.perf.tsx | 527.11 | 484.49
| ChatWithPopover.perf.tsx | 502.11 | 445.43
| ChatDuplicateMessages.perf.tsx | 472.07 | 422.86
| CustomToolbar.perf.tsx | 61.43 | 57.85
| SliderMinimal.perf.tsx | 21.2 | 19.96
| ListCommon.perf.tsx | 17.87 | 15.69
| EmbedMinimal.perf.tsx | 17.29 | 15.68
| DropdownMinimal.perf.tsx | 13.66 | 14.16
| AttachmentSlots.perf.tsx | 11.98 | 10.02
| CarouselMinimal.perf.tsx | 11.61 | 11.26
| SplitButtonMinimal.perf.tsx | 10.27 | 8.88
| CheckboxMinimal.perf.tsx | 8.68 | 7.43
| ProviderMinimal.perf.tsx | 6.3 | 6.58
| LoaderMinimal.perf.tsx | 5.97 | 6.02
| ButtonSlots.perf.tsx | 5.18 | 5
| ReactionMinimal.perf.tsx | 4.94 | 4.8
| InputMinimal.perf.tsx | 4.6 | 4.15
| ButtonMinimal.perf.tsx | 4.07 | 3.35
| AttachmentMinimal.perf.tsx | 3.99 | 4.33
| TextAreaMinimal.perf.tsx | 3.59 | 3.43
| DialogMinimal.perf.tsx | 2.7 | 2.48
| AvatarMinimal.perf.tsx | 2.5 | 2.27
| AlertMinimal.perf.tsx | 2.37 | 2.3
| MenuMinimal.perf.tsx | 2.3 | 2.16
| ItemLayoutMinimal.perf.tsx | 2.12 | 1.97
| MenuButtonMinimal.perf.tsx | 1.99 | 1.76
| LabelMinimal.perf.tsx | 1.94 | 1.63
| AccordionMinimal.perf.tsx | 1.83 | 1.62
| TreeMinimal.perf.tsx | 1.69 | 1.37
| HeaderSlots.perf.tsx | 1.63 | 1.63
| SegmentMinimal.perf.tsx | 1.37 | 1.32
| VideoMinimal.perf.tsx | 1.35 | 1.32
| HierarchicalTreeMinimal.perf.tsx | 1.35 | 1.39
| ChatMinimal.perf.tsx | 1.29 | 1.05
| FormMinimal.perf.tsx | 1.23 | 1.17
| ToolbarMinimal.perf.tsx | 1.23 | 1.46
| DividerMinimal.perf.tsx | 1.18 | 1.18
| TableMinimal.perf.tsx | 1.05 | 0.97
| GridMinimal.perf.tsx | 1.03 | 1.01
| IconMinimal.perf.tsx | 0.95 | 0.97
| RadioGroupMinimal.perf.tsx | 0.94 | 0.9
| StatusMinimal.perf.tsx | 0.87 | 0.73
| PopupMinimal.perf.tsx | 0.85 | 0.92
| ListMinimal.perf.tsx | 0.8 | 0.67
| ChatMinimal.perf.tsx | 1.29 | 1.05
| FlexMinimal.perf.tsx | 0.71 | 0.68
| HeaderMinimal.perf.tsx | 0.69 | 0.59
| ImageMinimal.perf.tsx | 0.64 | 0.5
| LayoutMinimal.perf.tsx | 0.62 | 0.61
| AnimationMinimal.perf.tsx | 0.5 | 0.48
| TextMinimal.perf.tsx | 0.47 | 0.5
| PortalMinimal.perf.tsx | 0.37 | 0.36
| RefMinimal.perf.tsx | 0.23 | 0.19
| BoxMinimal.perf.tsx | 0.21 | 0.19
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.
Please see comment: #2200 (comment)
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.
Cool, thanks for the diff. It looks like this isn't making a substantial improvement. Although many are a bit faster, some are actually slower too.
Let's see what happens with the immutable theme approach. Flamegrill can also help show you where cycles are being spent. You might want to compare approaches of hash/stringify with immutable and see where the time is going.
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.
I opened PR: https://github.com/microsoft/fluent-ui-react/pull/2226/files and reverted the changes in this PR. Left some comments, take a look when you can, and we can move the discussion there.
> | ||
<Input placeholder="Default input..." /> | ||
</Grid> | ||
{/* TODO: figure this out - changed because of restricting props styles to be only object :\ */} |
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.
Why do we require that the styles
prop be only an object?
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.
Because if we use mergeStyles
it is always the same function, hence resulting in wrong hash key (although the parameters are different). This is not an issue when we are passing the styles from parent => child, as by the time they get to the factories, they are already resolved. It's literally only for the styles prop... I don't have other idea of how to solve this at this moment..
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.
Hm, OK. Let's revisit. This one seems like a complex rabbit hole.
// const items = [ | ||
// { slots: { text: MenuItemText }, text: { id: 'blabla', children: 'Bla' } }, | ||
// { slots: { text: MenuItemText }, text: { id: 'blabla', children: 'Foo' } } | ||
// ]; |
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.
Yep, you should update this PR based on your proposal here.
|
||
/** | ||
* TODO: | ||
* 1) do we really need slots prop? |
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.
Do you mean keeping this in context only opposed to also having a slot?
<Provider theme={{
components: {
Button: {
slots: { icon: MyIcon },
}
}
}}
/>
Ignore the fact that I put this in
theme.components
. I just mean to show "top-level config via context" vs a prop.
vs
<Button slots={{ icon: MyIcon }} />
This would allow easy inline overrides of the slots, but is it necessary?
mergeComponentStylesThemeCache[themeHash] = Object.keys(mergeComponentStylesThemeCache).length | ||
} | ||
|
||
const hashVal = JSON.stringify({ ...hashObj, theme: mergeComponentStylesThemeCache[themeHash] }) |
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.
I think we should try a WeakMap here instead. You can use a complex object for the key in that case and avoid the hashing and JSON.stringify.
We want to cache on the theme object and a filtered set of component props, the props that are used in the styles. If the theme is the same, and the props used in the styles are the same, then we know we can return the cached result.
This way, we only have to merge, resolve, and render styles for new themes and prop pairs that we haven't seen before.
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.
I don't see how we can use WeakMap for the caching, as it is comparing based on the reference:
// theme1 and theme2 are basically the same object, but different references
const theme1 = {
backgroundColor: 'red',
color: 'white',
}
const theme2 = {
backgroundColor: 'red',
color: 'white',
}
const map = new WeakMap();
map.set(theme1, "Theme1");
if(map.has(theme2)) { // we would want this to return true, but it returns false
console.log("equal")
}
//adding new prop
theme1.fontSize = "14px"
if(map.has(theme1)) { // more over we definitelly don't want this to happen, but it does as it is the same reference
console.log("Still has theme1")
}
I understand that JSON.stringify is not that cheap and I tried the object-hash
package, but it is even slower... When comparing we have to be sure that the structure of the object are the same, not their reference. If I am missing something, or you have other suggestion let me know.
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.
Updating from offline chat:
The difference here is that theme1 === theme2
is lightning fast and simple to implement. The cost of deep checking if one object equals another or maintaining a hash/id algorithm will be much less performant.
So, let's shift the complexity of this problem from hashing/ids to ensuring immutable themes.
What is the status of this thing? Should we close it? |
DO NOT REVIEW YET