Proposal for makeStyles() next#16082
Conversation
layershifter
requested review from
assuncaocharles,
dzearing,
jurokapsiar,
ling1726,
miroslavstastny,
pompomon and
xugao
as code owners
|
|
||
| const useButtonStyles = makeStyles([ | ||
| [ | ||
| null, |
There was a problem hiding this comment.
To make the API a bit more intuitive would it make sense to create a builder kind of API ? either to build selector-styles pairs (decoupling building/making):
import { builder } from 'make-styles';
const rawStyles = builder
.addDefault({...styles}) // no need to declare null selector
.addSelector({text: false}, {...styles})
const useButtonStyles = makeStyles(rawStyles)
or makeStyles could use a builder pattern with a fluent API:
const useButtonStyles = makeStyles()
.addDefault({...styles}) // your null selector
.addSelector({text: false}, {...styles})
.make();
const buttonStylesMaker = makeStyles()
.addDefault({...styles}) // your null selector
.addSelector({text: false}, {...styles});
const useButtonStyles = buttonStylesMaker.make();
There was a problem hiding this comment.
This would also mean you could have project level style makers that could be reused ?
There was a problem hiding this comment.
null selector probably should become {}
There was a problem hiding this comment.
That's fair, do you think the above pattern is unnecessary in that case ?
There was a problem hiding this comment.
That's an interesting idea @ling1726 :) I could see how a builder could allow the hook to be a little more extendable and the api a little more explicit.
Slightly concerned deviating too far from css. It's already weird to be writing css in javascript for many people; deviating from normal css in js selectors by now having a matcher API might be another hurdle for adoption. Not dismissing the approach, though. Just need to chew on it a bit.
Regarding extendability of rules, in makeStyles I was considering something like extend as well:
Option 1, let extends property reference another hook that becomes injected:
const useButtonStyles = makeStyles( ... );
const useMyButtonStyles = makeStyles({
extends: useButtonStyles,
root: { ... },
icon: { ... }
});Option 2, let the hook have an extendable method:
const useMyButtonStyles = useButtonStyles.extend({
root: ...,
icon: ...
});Option 3, pass a hook into makeStyles and have it recognize a style hook and extract the rules:
const useMyButtonStyles = makeStyles(useButtonStyles, { ... });There was a problem hiding this comment.
Realized that option 3 is closest to how StyleX works for extending existing classes:
There was a problem hiding this comment.
It was covered in offline why we would prefer to use className as a plain property can be passed to any elements.
Another issue is related to overrides and their context, it's a real case in Avatar + Status.
const useStatusStyles = makeStyles();
function Status(props) {
return (
<div
className={useStatusStyles({ status: props.status }, props.className)}
/>
);
}const useAvatarStatusStyles = makeStyles();
function Avatar(props) {
// ⚠️ See usage
return <Status className={useAvatarStatusStyles({ square: props.square })} />;
}Avatar can be square, but Status cannot be. With the current implementation overrides will be resolved in Avatar and then passed to Status.
In "stylex":
- they should be resolved in
Statusand it means that it somehow should knowsquare - they should be resolved in
Avatarand it means that it has the same issue as the existing solution in v0 where we have sometimesclassNameandstyles
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: de2be83fbac3276be793cf311eaabfb337cc0c47 (build) |
| }), | ||
| ], | ||
|
|
||
| [{ state: 'success' }, tokens => ({ backgroundColor: tokens.colorScheme.green.background })], |
There was a problem hiding this comment.
Trying to pin down how this should work in extraction. Here's a scenario:
makeStyles([
[ null, { background: 'red' } ],
[ { primary: true }, { background: 'green' } ],
[ { state: 'info' }, { background: 'blue' } ]
]);In this case, we have 3 possible atomic css classes that will be hashed: let's refer to them as r, g, and b (though they'll be more like 123abc using a hash function.)
- If we're in a default state, className should be
r. - If primary is true, className should be
g. - If
stateisinfo, className should beb. - If
stateisinfoandprimaryis true, state wins and className should beb.
My initial extraction expectation was that this would be extracted like so:
// extract this stylesheet: `.r{background:red}.g{background:green}.b{background:blue}`.
// code is transformed into this:
```jsx
makeStyles([
[ null, 'r' ],
[ { primary: true }: 'b' ],
[ { state: 'info' }: 'g' ]
]);But I believe that means that we will also have to inject some javascript for rehydrating the classname hash map. Without priming the hash map, the class names will be joined, resulting in specificity to control the result.
Am I understanding this correctly?
There was a problem hiding this comment.
But I believe that means that we will also have to inject some javascript for rehydrating the classname hash map. Without priming the hash map, the class names will be joined, resulting in specificity to control the result.
Am I understanding this correctly?
A small correction, it will keep property name and we will perform a merge later based on it:
makeStyles([
[null, { background: "r" }],
[{ primary: true }, { background: "b" }],
[{ state: "info" }, { background: "g" }]
]);So for { primary: true } we will get:
[{ background: "r" }, { background: "b" }];Once we will shallow merge:
const classes = Object.values(
Object.assign({}, { background: "r" }, { background: "b" })
); // ["b"]
| ], | ||
|
|
||
| [ | ||
| { text: false }, |
There was a problem hiding this comment.
Is there a scenario where falsey matches are definitely needed? Can't the default state represent the default look and flags define variants off the default look?
And, if there is a scenario that requires them, how would we represent "enum doesn't equal value" scenarios?
There was a problem hiding this comment.
Is there a scenario where falsey matches are definitely needed?
To be honest, we are not 100% sure about it. It might be useful or not, our existing styles have a lot of negative conditions. I would also prefer to use default values and override them
layershifter
force-pushed
the
feat/make-styles-next
branch
from
3b09d8f to
3de3fad
Compare
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Perf comparison
Perf tests with no regressions
|
|
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 954ca4c:
|
khmakoto
left a comment
khmakoto
left a comment
There was a problem hiding this comment.
Left 2 suggestions but otherwise looks good to me!
Co-authored-by: Xu Gao <xugao0131@hotmail.com>
Co-authored-by: Miroslav Stastny <mistastn@microsoft.com>
…fluentui into feat/make-styles-next
Co-authored-by: Xu Gao <xugao0131@hotmail.com>
…fluentui into feat/make-styles-next
|
🎉 Handy links: |
9 participants
This PR contains a new version of
makeStylesthat we are evaluating last weeks.😕 Problems
Perf is a problem
Styles are computed for each part of each component, including duplicate calculations when there are multiple instances of the same component on the page. Even with caching it slow compared to native CSS.
Predictable style overrides
Current version of
makeStyles()is heavily inspired by Material UI and depends on file evaluation order which makes complicated to use it with React Suspense, for example: https://codesandbox.io/s/kind-davinci-xvwxgThere is also no way to solve CSS specificity issues for styles overrides.
Improve Developer Experience
For Northstar: Style files are currently hard to read and manage due to their complexity. The complexity is caused by heavily nested conditionals and the use of abstractions (like helper functions). Styles also decoupled from components that caused an additional indirection.
📜 Key notes
The new iteration of
makeStyles()splits the expensive part (processing styles, generating classnames) and the cheap part (merging classnames), expensive part can be done build time. Code also intentionally separated to highlight required part ofmakeStyles()and runtime. A production version ofmakeStyles()currently is less than 200 LoC.It uses the similar side effect approach as in
useCSShook and Emotion (#14470) (passing down classnames but referrencing a dictionary when merging).We use hash based atomic classnames, this makes deterministic (vs Fela is sequential = non-deterministic), it simplifies support server side rendering.
Tokens (theme, siteVariables) are injected as CSS variables, can easily fallback to runtime evaluation in IE 11 without any change required on component/overrides side. However, this will require bundling of runtime part.
📦 Current API proposal
Anything can change based on feedback, but current API is shown below:
Why selectors and styles are separated?
It removes any conditions in styles and and allows easily transform them to CSS.
Why selectors are functions?
This allows to match styles in a single loop i.e. the best performance option that we have.
I also evaluated matchers approach (microsoft/fluent-ui-react#1301), but we need have to compare there two objects:
Another idea that was not enough performant is to transform these matchers to bitmasks. It also was slower as we need to transform user's input masks which gives us a second loop 🐌:
It also made code complicated for understanding and debugging.
🏁 Performance
Based on modified perf test from
react-native-web. A deep mount of 50 components, contains styles overrides (BoxoverridesView).Current version of makeStyles was also measured and it's fast as native styles.
Compare to Emotion
Compare to native CSS
Another perf test compares what we have now in Fluent UI N*.
ImageMinimal.perf.tsxrenders 5kImagecomponents, the simplest component with caching enabled in FelaAvatarMinimal.perf.tsxrenders 5kAvatarcomponents (rendersStatus&Label), show what happens when caching breaks in Felano styles at all i.e. unstyled components
makeStyles: build time, CSS variables
Current styling system in Northstar
Current PR ships only runtime part and some tests for it. Build time transform are next on the list.
@fluentui/make-stylesintentionally is private to avoid publishing.