Skip to content

Conversation

@bsunderhus
Copy link
Contributor

@bsunderhus bsunderhus commented Aug 29, 2022

As there's no easy way to force state in Typescript to be only the properties you've declared, it is possible to create a superset of state by spreading props on it. This creates unmapped properties on state being passed down the line, in the remote case state is spread on an element (hope this doesn't happen) this will generate undesired attributes on that element.

A better alternative is to simply not spread ...props into state, instead redeclare properties required by state.

Effective changes:

  1. stops spreading props on state
  2. makes MenuState types stricter

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 29, 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 2ff33bc:

Sandbox Source
@fluentui/react 8 starter Configuration
@fluentui/react-components 9 starter Configuration

@fabricteam
Copy link
Collaborator

fabricteam commented Aug 29, 2022

📊 Bundle size report

Package & Exports Baseline (minified/GZIP) PR Change
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
188.818 kB
51.901 kB
189.299 kB
51.987 kB
481 B
86 B
react-menu
Menu (including children components)
115.697 kB
35.316 kB
116.178 kB
35.418 kB
481 B
102 B
react-menu
Menu (including selectable components)
118.896 kB
35.806 kB
119.377 kB
35.904 kB
481 B
98 B
Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-components
react-components: FluentProvider & webLightTheme
33.259 kB
10.951 kB
🤖 This report was generated against 0174dfc96d5ea925268fb3f51efaa6373ecabcc6

@size-auditor
Copy link

size-auditor bot commented Aug 29, 2022

Asset size changes

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

Baseline commit: 0174dfc96d5ea925268fb3f51efaa6373ecabcc6 (build)

@fabricteam
Copy link
Collaborator

fabricteam commented Aug 29, 2022

Perf Analysis (@fluentui/react-components)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 1301 1299 5000
Button mount 959 946 5000
FluentProvider mount 1572 1567 5000
FluentProviderWithTheme mount 629 630 10
FluentProviderWithTheme virtual-rerender 592 586 10
FluentProviderWithTheme virtual-rerender-with-unmount 626 638 10
MakeStyles mount 1922 1925 50000
SpinButton mount 2591 2480 5000

@layershifter
Copy link
Member

@bsunderhus FYI CI is failing:

image

@bsunderhus
Copy link
Contributor Author

@bsunderhus FYI CI is failing:

image

I'm on it @layershifter , the fact that this doesn't pass this easily is one more argument to stop spreading props all over the place.

@bsunderhus bsunderhus marked this pull request as draft August 29, 2022 08:10
@bsunderhus bsunderhus force-pushed the react-menu-stop-spreading-props-on-state branch from d809283 to f64ff62 Compare August 29, 2022 08:23
@bsunderhus bsunderhus marked this pull request as ready for review August 29, 2022 09:12
@bsunderhus bsunderhus force-pushed the react-menu-stop-spreading-props-on-state branch from f64ff62 to 2ff33bc Compare August 31, 2022 16:41
Copy link
Contributor

@ling1726 ling1726 left a comment

Choose a reason for hiding this comment

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

yeah stricter typings for this are 🚀

@bsunderhus bsunderhus merged commit a105236 into microsoft:master Sep 5, 2022
@bsunderhus bsunderhus deleted the react-menu-stop-spreading-props-on-state branch September 5, 2022 17:01
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.

4 participants