fluentui icon indicating copy to clipboard operation
fluentui copied to clipboard

Published 20 hours ago • verified publisher icon microsoft

feat(react-jsx-runtime): implements next steps (option D)

Open bsunderhus opened this issue 2 years ago • 8 comments
trafficstars

Previous Behavior

New Behavior

Related Issue(s)

  • Fixes #

bsunderhus avatar May 03 '23 07:05 bsunderhus

📊 Bundle size report

Package & Exports Baseline (minified/GZIP) PR Change
react-accordion
Accordion (including children components)		 88.516 kB
26.795 kB		 88.501 kB
26.845 kB		 -15 B
50 B
react-alert
Alert		 93.549 kB
22.537 kB		 93.74 kB
22.643 kB		 191 B
106 B
react-avatar
Avatar		 57.797 kB
15.091 kB		 57.755 kB
15.073 kB		 -42 B
-18 B
react-avatar
AvatarGroup		 15.646 kB
6.298 kB		 15.822 kB
6.382 kB		 176 B
84 B
react-avatar
AvatarGroupItem		 73.973 kB
19.582 kB		 73.938 kB
19.578 kB		 -35 B
-4 B
react-badge
Badge		 23.555 kB
7.256 kB		 23.421 kB
7.226 kB		 -134 B
-30 B
react-badge
CounterBadge		 24.457 kB
7.559 kB		 24.329 kB
7.529 kB		 -128 B
-30 B
react-badge
PresenceBadge		 32.135 kB
8.423 kB		 32.02 kB
8.392 kB		 -115 B
-31 B
react-button
Button		 36.742 kB
9.5 kB		 36.788 kB
9.504 kB		 46 B
4 B
react-button
CompoundButton		 43.896 kB
10.98 kB		 43.946 kB
10.975 kB		 50 B
-5 B
react-button
MenuButton		 41.427 kB
10.836 kB		 41.482 kB
10.845 kB		 55 B
9 B
react-button
SplitButton		 49.649 kB
12.42 kB		 49.706 kB
12.463 kB		 57 B
43 B
react-button
ToggleButton		 55.024 kB
11.436 kB		 55.065 kB
11.44 kB		 41 B
4 B
react-card
Card - All		 88.716 kB
25.114 kB		 88.696 kB
25.099 kB		 -20 B
-15 B
react-card
Card		 83.651 kB
23.658 kB		 83.525 kB
23.659 kB		 -126 B
1 B
react-card
CardFooter		 9.193 kB
3.892 kB		 9.06 kB
3.863 kB		 -133 B
-29 B
react-card
CardHeader		 11.089 kB
4.588 kB		 11.003 kB
4.551 kB		 -86 B
-37 B
react-card
CardPreview		 9.998 kB
4.24 kB		 9.873 kB
4.213 kB		 -125 B
-27 B
react-checkbox
Checkbox		 34.5 kB
10.878 kB		 34.405 kB
10.868 kB		 -95 B
-10 B
react-combobox
Combobox (including child components)		 87.735 kB
28.243 kB		 87.65 kB
28.212 kB		 -85 B
-31 B
react-combobox
Dropdown (including child components)		 86.074 kB
27.848 kB		 85.989 kB
27.822 kB		 -85 B
-26 B
react-components
react-components: Button, FluentProvider & webLightTheme		 64.899 kB
17.91 kB		 64.965 kB
17.944 kB		 66 B
34 B
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover		 206.425 kB
57.914 kB		 206.488 kB
57.953 kB		 63 B
39 B
react-components
react-components: FluentProvider & webLightTheme		 36.132 kB
11.954 kB		 36.302 kB
12.019 kB		 170 B
65 B
react-datepicker-compat
DatePicker Compat		 222.56 kB
59.204 kB		 222.486 kB
59.165 kB		 -74 B
-39 B
react-dialog
Dialog (including children components)		 92.076 kB
27.492 kB		 91.961 kB
27.484 kB		 -115 B
-8 B
react-divider
Divider		 17.441 kB
6.349 kB		 17.303 kB
6.306 kB		 -138 B
-43 B
react-field
Field		 18.9 kB
7.083 kB		 18.844 kB
7.045 kB		 -56 B
-38 B
react-image
Image		 11.514 kB
4.619 kB		 11.691 kB
4.709 kB		 177 B
90 B
react-infobutton
InfoButton		 130.121 kB
39.785 kB		 130.029 kB
39.767 kB		 -92 B
-18 B
react-infobutton
InfoLabel		 133.586 kB
40.852 kB		 133.523 kB
40.868 kB		 -63 B
16 B
react-input
Input		 24.183 kB
7.772 kB		 24.047 kB
7.716 kB		 -136 B
-56 B
react-label
Label		 10.139 kB
4.231 kB		 10.005 kB
4.207 kB		 -134 B
-24 B
react-link
Link		 12.339 kB
5.105 kB		 12.52 kB
5.195 kB		 181 B
90 B
react-menu
Menu (including children components)		 130.848 kB
39.946 kB		 130.774 kB
39.9 kB		 -74 B
-46 B
react-menu
Menu (including selectable components)		 133.832 kB
40.479 kB		 133.666 kB
40.421 kB		 -166 B
-58 B
react-persona
Persona		 64.718 kB
17.012 kB		 64.752 kB
16.989 kB		 34 B
-23 B
react-popover
Popover		 117.083 kB
36.122 kB		 117.278 kB
36.189 kB		 195 B
67 B
react-progress
ProgressBar		 13.891 kB
5.482 kB		 13.757 kB
5.438 kB		 -134 B
-44 B
react-provider
FluentProvider		 18.079 kB
6.713 kB		 18.249 kB
6.779 kB		 170 B
66 B
react-radio
Radio		 27.404 kB
8.722 kB		 27.313 kB
8.713 kB		 -91 B
-9 B
react-radio
RadioGroup		 11.326 kB
4.743 kB		 11.503 kB
4.821 kB		 177 B
78 B
react-select
Select		 25.373 kB
8.826 kB		 25.241 kB
8.785 kB		 -132 B
-41 B
react-slider
Slider		 34.322 kB
11.099 kB		 34.209 kB
11.057 kB		 -113 B
-42 B
react-spinbutton
SpinButton		 34.121 kB
10.421 kB		 33.981 kB
10.376 kB		 -140 B
-45 B
react-spinner
Spinner		 21.327 kB
7.015 kB		 21.247 kB
7.01 kB		 -80 B
-5 B
react-switch
Switch		 29.924 kB
9.342 kB		 29.834 kB
9.349 kB		 -90 B
7 B
react-table
DataGrid		 150.868 kB
41.518 kB		 150.899 kB
41.587 kB		 31 B
69 B
react-table
Table (Primitives only)		 45.111 kB
12.567 kB		 45.191 kB
12.568 kB		 80 B
1 B
react-table
Table as DataGrid		 133.356 kB
34.002 kB		 133.481 kB
34.027 kB		 125 B
25 B
react-table
Table (Selection only)		 79.125 kB
19.379 kB		 79.248 kB
19.448 kB		 123 B
69 B
react-table
Table (Sort only)		 78.455 kB
19.187 kB		 78.578 kB
19.241 kB		 123 B
54 B
react-tags
Tag		 22.004 kB
7.93 kB		 21.923 kB
7.888 kB		 -81 B
-42 B
react-text
Text - Default		 12.527 kB
4.963 kB		 12.705 kB
5.051 kB		 178 B
88 B
react-text
Text - Wrappers		 15.677 kB
5.284 kB		 15.855 kB
5.368 kB		 178 B
84 B
react-textarea
Textarea		 27.686 kB
9.126 kB		 27.53 kB
9.094 kB		 -156 B
-32 B
react-tooltip
Tooltip		 47.119 kB
16.528 kB		 46.956 kB
16.487 kB		 -163 B
-41 B
Unchanged fixtures
Package & Exports Size (minified/GZIP)
global-context
createContext		 510 B
330 B
global-context
createContextSelector		 537 B
342 B
react-overflow
hooks only		 11.206 kB
4.266 kB
react-portal
Portal		 11.676 kB
4.31 kB
react-portal-compat
PortalCompatProvider		 6.473 kB
2.196 kB
react-positioning
usePositioning		 24.249 kB
8.856 kB
react-utilities
SSRProvider		 180 B
159 B
🤖 This report was generated against 65f0125dd6a85c3c772d63ad3350561af77addbc

fabricteam avatar May 03 '23 07:05 fabricteam

Asset size changes

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

Baseline commit: 65f0125dd6a85c3c772d63ad3350561af77addbc (build)

size-auditor[bot] avatar May 03 '23 07:05 size-auditor[bot]

Perf Analysis (@fluentui/react-components)

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 578 599 5000 Possible regression
Button mount 294 302 5000 Possible regression
InfoButton mount 17 17 5000 Possible regression
SpinButton mount 1279 1334 5000 Possible regression
All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 578 599 5000 Possible regression
Button mount 294 302 5000 Possible regression
Field mount 1009 1051 5000
FluentProvider mount 659 669 5000
FluentProviderWithTheme mount 78 85 10
FluentProviderWithTheme virtual-rerender 72 72 10
FluentProviderWithTheme virtual-rerender-with-unmount 76 76 10
InfoButton mount 17 17 5000 Possible regression
MakeStyles mount 904 875 50000
Persona mount 1601 1654 5000
SpinButton mount 1279 1334 5000 Possible regression

fabricteam avatar May 03 '23 07:05 fabricteam

It would be good to summarize changes required from consumers/developers to upgrade to a new approach

layershifter avatar May 04 '23 09:05 layershifter

I am in favor of Option D (this PR)

  • it's less changes ➡️ faster upgrade path
  • smaller bundle size as .props can't be minified

Anyway, before we will do any changes we need to ensure that a new release with these changes will not break consumers

layershifter avatar May 04 '23 09:05 layershifter

The root problem is more than just being a required slot problem @behowell. There are a few problems involved here.

getNativeElementProps is harmful to our typings

is basically a blackhole that allows you to do so many wrong things

Here's an example:

In TreeItem component we have a handler that is used for both cases of focus and mouseover


const handleActionsVisible = useEventCallback((event: React.FocusEvent) => {/**/})

return {
  components: {
    root: 'div',
  },
  root: getNativeElementProps(as, {
    onMouseOver: handleActionsVisible,
    onFocus: handleActionsVisible,
  }),
};

Those are 2 completely different types of events React.FocusEvent | React.MouseEvent, but by a mistake I've only added React.FocusEvent as the argument type, and since getNativeElementProps is not very strict about type inferences (there's none) this works just fine and I got no error on TS side.

We remove ref from the primary slot type on ComponentProps

So if we properly type root as ExtractSlotProps<Slots['root']> we can get that type, but that's a little verbose.

{
    // this works, is just too verbose and I'm assuming nobody will do this,
    // and instead they'll just use `DialogActionsProps` which is wrong.
    root: slot<ExtractSlotProps<DialogActionsSlots['root']>>({
      shorthand: { ...props, ref },
      required: true,
      elementType: as,
    }),
    position,
    fluid,
  }

It's not possible to validate by types the splitting of props that are supposed to go to the root slot

This is the reason why we can't simply ditch getNativeElementProps


  const { position = 'end', fluid = false, as = 'div' } = props;
  return {
    root: slot<ExtractSlotProps<DialogActionsSlots['root']>>({
      // spreading props here is a problem, and here's the worst part: TS doesn't get that 🚨
      shorthand: { ...props, ref },
      required: true,
      elementType: as,
    }),
    position,
    fluid,
  };

we have to repeat elementType twice (not a big deal)

{
    root: slot<ExtractSlotProps<DialogActionsSlots['root']>>({

      shorthand: getNativeElementProps(as /* 1 */, { ...props, ref }),
      required: true,
      elementType: as, /* 2 */
    }),
    position,
    fluid,
  }

Solution?

We should probably provide a slot method that makes the use of getNativeElementProps internal to it to make sure those edge cases are properly done (I guess I'll have to investigate component with primary slot different than root just to be sure how to make this work properly in other edge cases), otherwise one of the problems above will always be a problem

Another possible solution would be to refactor getNativeElementProps to be properly typed (but I'm afraid of doing that as it's a wide spread method)

If we keep everything as it is, this is how root slots should look like:

export const useDialogActions_unstable = (
  props: DialogActionsProps,
  ref: React.Ref<HTMLDivElement>,
): DialogActionsState => {
  const { position = 'end', fluid = false, as = 'div' } = props;
  return {
    root: slot<ExtractSlotProps<DialogActionsSlots['root']>>({
      // keep in mind also that this is only enforcing the return type of `getNativeElementProps`,
     // the second argument to `getNativeElementProps` is still quite harmful to our
     // typings as you can basically pass anything you want there
      shorthand: getNativeElementProps<ExtractSlotProps<DialogActionsSlots['root']>>(as, { ...props, ref }),
      required: true,
      elementType: as,
    }),
    position,
    fluid,
  };
};

bsunderhus avatar May 18 '23 08:05 bsunderhus

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 72baa4bb624c2d75229174c9e68ae31a784edb4b:

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

codesandbox-ci[bot] avatar May 23 '23 12:05 codesandbox-ci[bot]

🕵 fluentuiv9 No visual regressions between this PR and main

fabricteam avatar May 29 '23 21:05 fabricteam