Fluent Provider - convert to useLayoutEffect approach with useInsertionEffect when possible#24061
Conversation
...ct-components/react-provider/src/components/FluentProvider/useFluentProviderThemeStyleTag.ts
Outdated
Show resolved
Hide resolved
📊 Bundle size report🤖 This report was generated against d08b249e242ea328bb33eb0f203850f461144342 |
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 1652 | 1608 | 5000 | |
| Button | mount | 1071 | 1062 | 5000 | |
| FluentProvider | mount | 2152 | 1957 | 5000 | |
| FluentProviderWithTheme | mount | 692 | 699 | 10 | |
| FluentProviderWithTheme | virtual-rerender | 638 | 663 | 10 | |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 697 | 702 | 10 | |
| MakeStyles | mount | 2248 | 2209 | 50000 | |
| SpinButton | mount | 3285 | 3294 | 5000 |
|
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 01a5a7a:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: d08b249e242ea328bb33eb0f203850f461144342 (build) |
change/@fluentui-react-provider-f7af8760-705e-4c2e-add0-9746ee74b04b.json
Outdated
Show resolved
Hide resolved
...ct-components/react-provider/src/components/FluentProvider/useFluentProviderThemeStyleTag.ts
Show resolved
Hide resolved
...ct-components/react-provider/src/components/FluentProvider/useFluentProviderThemeStyleTag.ts
Show resolved
Hide resolved
| if (styleTag.current) { | ||
| insertSheet(styleTag.current, rule); | ||
| return () => { | ||
| styleTag.current?.remove(); |
There was a problem hiding this comment.
| styleTag.current?.remove(); | |
| styleTag.current.remove(); |
We are checking that styleTag.current is defined few lines above so it's not needed, right?
There was a problem hiding this comment.
i thought so as well, but linting was throwing an error
There was a problem hiding this comment.
I think the error was because it's a ref, so styleTag.current could theoretically be changed/nulled by other code before the cleanup function runs. In this case, you're not using the styleTag ref outside of the effect anyways, so you could delete the ref and make it a local const instead.
useInsertionEffect(() => {
const styleTag = createStyleTag(targetDocument, styleTagId);
if (styleTag) {
insertSheet(styleTag, rule);
return () => {
styleTag.remove();
};
}
}, [styleTagId, targetDocument, rule]);
...ct-components/react-provider/src/components/FluentProvider/useFluentProviderThemeStyleTag.ts
Show resolved
Hide resolved
…74b04b.json Co-authored-by: Makoto Morimoto <Humberto.Morimoto@microsoft.com>
…Provider/useFluentProviderThemeStyleTag.ts Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
…Provider/useFluentProviderThemeStyleTag.ts Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>


fixes #23625
Two things happening here: