-
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: use mergeVariablesOverrides() to avoid cache breaks when variables are not passed #13374
Conversation
…les are not passed
…ithub.com/microsoft/fluentui into perf/use-simple-merge-vars
mergeComponentVariables() is always creating a function even if the arguments are undefined we have this temporary | ||
fix in place to avoid creating empty function because it is breaking caching we should either fix | ||
mergeComponentVariables(), or handle this in a more generic way. | ||
*/ |
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 we still need this 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.
I would say yes to provide a context why this is a solution
fix in place to avoid creating empty function because it is breaking caching we should either fix | ||
mergeComponentVariables(), or handle this in a more generic way. | ||
*/ | ||
export default function mergeVariablesOverrides( |
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.
should we add this under @fluentui/styles
with the other merge functions?
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 have there any preference, however I placed it here as it's more attached to component's specifics
}); | ||
}); | ||
}; | ||
|
||
const renderHeader = () => { | ||
if (!header) { |
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.
Should we really remove this check?
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.
Yup, as .create()
call few lines below handles this:
fluentui/packages/fluentui/react-northstar/src/components/Table/Table.tsx
Lines 111 to 124 in e5adec2
const renderHeader = () => { | |
if (!header) { | |
return null; | |
} | |
const headerRowProps = { | |
header: true, | |
compact, | |
className: tableSlotClassNames.header, | |
} as TableRowProps; | |
const overrideProps = handleVariablesOverrides(variables); | |
return TableRow.create(header, { |
...overrideProps, | ||
}), | ||
defaultProps: () => getA11yProps('cell', {}), | ||
overrideProps: predefinedProps => ({ |
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.
👍
Hello @layershifter! Because this pull request has the Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 8 hours, a condition that will be fulfilled in about 7 hours 42 minutes. No worries though, I will be back when the time is right! 😉 p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Perf comparison
Perf tests with no regressions
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: e5adec2a91f1b920d1b0a2ab5e29828f2a7f4c30 (build) |
…les are not passed (microsoft#13374) * perf: use mergeVariablesOverrides() to avoid cache breaks when variables are not passed * add changelog entry * fix ts issue
Pull request checklist
$ yarn change
Description of changes
Fixes an issues that we have found with improved telemetry, see #13346 (comment).
This PR introduces
mergeVariablesOverrides()
which we already uses inAttachment
to avoid creation of variables functions even if there is no overrides as that breaks caching.Focus areas to test
(optional)