Skip to content
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

Fluent Provider - convert to useLayoutEffect approach with useInsertionEffect when possible #24061

Merged
merged 8 commits into from
Jul 26, 2022

Conversation

micahgodbolt
Copy link
Member

@micahgodbolt micahgodbolt commented Jul 25, 2022

fixes #23625

Two things happening here:

  1. To support React 18 we need to move to a symmetrical approach where we use useInsertionEffect to both insert the style and remove the style
  2. Rather than splitting the logic for 16 vs 18, after a discussion with Shift, we are also changing the default logic to use useLayoutEffect rather than the in-render insertion. This mirrors how emotionjs currently injects styles

@fabricteam
Copy link
Collaborator

fabricteam commented Jul 25, 2022

📊 Bundle size report

Package & Exports Baseline (minified/GZIP) PR Change
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
189.057 kB
52.092 kB
189.07 kB
52.088 kB
13 B
-4 B
react-components
react-components: FluentProvider & webLightTheme
31.883 kB
10.516 kB
32.036 kB
10.577 kB
153 B
61 B
react-provider
FluentProvider
14.76 kB
5.596 kB
14.913 kB
5.658 kB
153 B
62 B

🤖 This report was generated against d08b249e242ea328bb33eb0f203850f461144342

@fabricteam
Copy link
Collaborator

fabricteam commented Jul 25, 2022

Perf Analysis (@fluentui/react-components)

No significant results to display.

All results

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

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 25, 2022

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:

Sandbox Source
@fluentui/react 8 starter Configuration
@fluentui/react-components 9 starter Configuration
frosty-meitner-pykce3 Issue #23625

@size-auditor
Copy link

size-auditor bot commented Jul 25, 2022

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: d08b249e242ea328bb33eb0f203850f461144342 (build)

if (styleTag.current) {
insertSheet(styleTag.current, rule);
return () => {
styleTag.current?.remove();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
styleTag.current?.remove();
styleTag.current.remove();

We are checking that styleTag.current is defined few lines above so it's not needed, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i thought so as well, but linting was throwing an error

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]);

micahgodbolt and others added 3 commits July 26, 2022 07:59
…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>
@micahgodbolt micahgodbolt merged commit 76f5c61 into microsoft:master Jul 26, 2022
@micahgodbolt micahgodbolt deleted the fluent-provider-r18 branch July 26, 2022 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: StrictMode breaks v9 theming
9 participants