Conversation
kuzhelov
requested review from
dvdzkwsk,
layershifter,
levithomason,
miroslavstastny and
mnajdova
as code owners
Generated by 🚫 dangerJS |
Codecov Report
@@ Coverage Diff @@
## master #989 +/- ##
==========================================
+ Coverage 81.15% 81.16% +0.01%
==========================================
Files 673 675 +2
Lines 8654 8691 +37
Branches 1528 1529 +1
==========================================
+ Hits 7023 7054 +31
- Misses 1616 1622 +6
Partials 15 15
Continue to review full report at Codecov.
|
| }) | ||
| } | ||
|
|
||
| const handledProps = Object.keys(boxPropTypes) |
There was a problem hiding this comment.
maybe move to common? it took me a while to understand where it's used
| const { as: ElementType, theme } = props | ||
|
|
||
| if (!theme) { | ||
| return withThemeContext(theme => |
There was a problem hiding this comment.
NIT: inline withThemeContext? it's only used once:
if (theme) {
return render({
ElementType,
props,
renderer: theme.renderer,
})
}
return (
<FelaTheme>
{theme => render({
ElementType,
props,
renderer: theme.renderer,
})}
</FelaTheme>
)|
|
||
| const unhandledProps = getUnhandledProps({ handledProps }, props) | ||
| const classes = { | ||
| root: renderer.renderRule(callable(styles), props), |
There was a problem hiding this comment.
I guess the main optimization is somewhere here, can you explain in a few words how it's achieved?
There was a problem hiding this comment.
note that here we imply that styles is an already prepared object, so that we don't need to
- consume theme styles, merge them
- merge theme variables
- resolve styles with merged variables
The thing we are requiring by optimised box is set of already resolved styles - and this is, essentially, the thing that we always have in Stardust code
| > | ||
| {childrenExist(children) ? children : content} | ||
| </ElementType> | ||
| const Box: React.FunctionComponent<BoxProps> & { create: Function } = props => { |
There was a problem hiding this comment.
nit: React.FunctionComponentReact.FC
There was a problem hiding this comment.
would rather leave it as it is, due to its readability
There was a problem hiding this comment.
Please avoid all abbreviations in the repo. Code is for humans to read. Let minifiers and uglifiers make it small and unreadable.
| @@ -0,0 +1,9 @@ | |||
| const getDisplayName = (Component: React.ReactType) => { | |||
There was a problem hiding this comment.
where is this method used?
There was a problem hiding this comment.
this a merge artefact, thank you!
| export { default as Avatar, AvatarProps } from './components/Avatar/Avatar' | ||
|
|
||
| export { default as Box, BoxProps } from './components/Box/Box' | ||
| export { default as Box, BoxProps } from './components/Box/BoxHeavy' |
There was a problem hiding this comment.
why do we export BoxHeavy and what does it bring in addition to the new Box that we still need it?
There was a problem hiding this comment.
the main reason is mentioned in the description:
To prevent breaking changes, this PR has preserved original
Boxcomponent's functionality exported from Stardust.
Essentially, we would like to prevent Stardust from breaking changes to the clients that might already used theme-based styling approach to Box. Yes, while Box theme-based styles are not introduced anywhere in Stardust, it might be the case that client had already introduced some style overrides that target Box component at places of custom client's usage.
Consider the following example:
// this might be an approach taken to circumvent current Stardust problem
// of positioning support
<Box variables={{ absolutePositioned: true }}>
{someContent}
</Box>with client's styles
export const boxStyles = {
root: ({ variables }) => ({
variables.absolutePositioned && {
position: absolute
}
})Note that in that case it is always the case that client won't like to target inner Stardust Box elements - the intent is to introduce styles to only those boxes that are used on client side. If we would export light version of Box that doesn't support theme-based styling, then breaking change will be introduced to the client. However, if we would export the same Box component as before, there won't be any breaking changes happen
|
I think the solution here is too complex. We don't want to make optimizations that involve forking or deviating individual components. We also don't want to introduce any new APIs or ideas or require any refactoring in our optimizations. We should focus on removing complexity and special cases and put a hard stop on adding any special cases, like a light weight Box. Improvements made should be first be focused on core pieces such as mergeThemes, factories, UIComponent, ProviderConsumer, etc. Areas from which all other abstractions are derived. If these hot paths are optimized, every component will benefit drastically. Example, just by |
|
closing as this strategy but based on static API for React context was implemented for the entire Stardust library: #1163 |
5 participants
Approach
To prevent breaking changes, this PR has preserved original
Boxcomponent's functionality exported from Stardust.At the same time, lightweight version of
Boxcomponent was introduced, all the common pieces between these two were reused and placed in thecommon.tsfile. While these two components have the same API and, in most cases, are interchangeable, the reason previous version ofBoxwas preserved is that lightweightBoxdoesn't support style customizations via theming.While this
theme-based stylessupport forBoxis not used in Stardust anywhere, however, it might be used by Stardust clients. Eventually I might see it being organised the following way:Boxwas used), and will have minimal abstraction costsWith changes made in this PR Stardust consumes light-weight
Boxcomponent internally.Perf Numbers (from
yarn perf)List with 10 elements, median from 10x50 runs
Box- 67msBox- 58ms (~13%)ItemLayout- 57ms (~14%)List with 50 elements, median from 10x50runs
Box- 235msBox- 200ms (~14%)ItemLayout- 203ms (~13%)Opened questions
BoxandBoxHeavyvsBoxLightandBoxBoxandBoxHeavy), to make it clear from React Debug Tools that unoptimized Box is used - should we use the same instead for both?Areas for potential improvements
Flexrendering logicFlexstyles shouldn't be customized by themeaccessibility, etcmarginsinstead ofFlex.Gapelement - will allow to gain ~5% of perf improvementthemecould be passed as prop