-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf: add variables caching to v0 #12073
Conversation
); | ||
}); | ||
|
||
if (!hasOnlyBooleanVariables) { |
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.
We should not throw here, we should just not cache for this instance. We are not throwing in the other cases of caching, so we shouldn't here too. We cannot guarantee that all usages of variables are booleans in a project.
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.
During offline conversation we discovered issues and I changed the implementation to avoid throwing errors.
@@ -36,7 +36,8 @@ const defaultContext: StylesContextValue<{ renderRule: RendererRenderRule }> = { | |||
performance: { | |||
enableSanitizeCssPlugin: process.env.NODE_ENV !== 'production', | |||
enableStylesCaching: true, | |||
enableVariablesCaching: true | |||
enableVariablesCaching: true, | |||
enableHardVariablesCaching: false |
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 not enableBooleanVariablesCaching
?
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.
Renamed, it's definitely sounds better
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: fafbe44a2bb89af8aa06496b6f0f9c5d15a2500c (build) |
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Perf comparison
All perf tests
|
? `${displayName}:${JSON.stringify(stylesProps)}${styleParam.rtl}${styleParam.disableAnimations}` | ||
: ''; | ||
const propsCacheKey = cacheEnabled ? JSON.stringify(stylesProps) : ''; | ||
const variablesCacheKey = cacheEnabled && performance.enableBooleanVariablesCaching ? JSON.stringify(variables) : ''; |
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.
For the props it was not problematic that the props order would change, because we are always mapping them in the same order. On the other hand for the variables we cannot guarantee that, as those are purely users input. I propose to always order them alphabetically and than we can safely use JSON.stringify
, or clearly state somewhere that those should always be in the same order...
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.
Added this to Limitations section in PR description
expect(() => resolveStyles(options, resolvedVariables)).toThrowError(/Please check your "performance" settings on "Provider"/); | ||
}); | ||
|
||
test('when enabled only plain objects can be passed as "variables"', () => { |
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.
Are these last two tests still valid?
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, I changed their contents and updated titles
…ithub.com/OfficeDev/office-ui-fabric-react into feat/styles-hard-variables
Pull request checklist
$ yarn change
Description of changes
WAT? 😱
This PR adds
enableBooleanVariablesCaching
which is disabled by default. It allows to handle styles caching for cases when boolean variables are used:Limitations 💔
We use
JSON.stringify()
for the implementation and it means that boolean variables should go in the same order, i.e.Button
s below will create different cache entries:To avoid this the order should be always the same, proposed solution for customers: enforce the alphabetic order via ESLint rules.
Microsoft Reviewers: Open in CodeFlow