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

chore(web-components): update web types for TS 4.9 #31807

Closed

Conversation

radium-v
Copy link
Contributor

@radium-v radium-v commented Jun 24, 2024

Previous Behavior

For the @fluentui/web-components package, we use @types/web to work with modern browser features that older versions of TypeScript aren't familiar with. The way that this types package needs to be configured depends on the TypeScript version in use.

Additionally, certain ES2023 core features aren't available in TypeScript 4.9. The browsers we target all support findLast and findLastIndex, but TS 4.9 can't utilize ES2023 types.

New Behavior

  • Hoists the @types/web package to the root as a devDependency
  • Removes duplicate properties from our tsconfig files
  • Adds a es2023-shim types package to support findLast and findLastIndex

@fabricteam
Copy link
Collaborator

fabricteam commented Jun 24, 2024

Perf Analysis (@fluentui/react-components)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 650 613 5000
Button mount 301 293 5000
Field mount 1109 1112 5000
FluentProvider mount 715 714 5000
FluentProviderWithTheme mount 90 92 10
FluentProviderWithTheme virtual-rerender 40 35 10
FluentProviderWithTheme virtual-rerender-with-unmount 74 72 10
MakeStyles mount 853 853 50000
Persona mount 1766 1739 5000
SpinButton mount 1373 1366 5000
SwatchPicker mount 1645 1678 5000

@fabricteam
Copy link
Collaborator

fabricteam commented Jun 24, 2024

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
PortalMinimalPerf.default 90 81 1.11:1
AlertMinimalPerf.default 163 150 1.09:1
AttachmentMinimalPerf.default 88 81 1.09:1
TextMinimalPerf.default 204 188 1.09:1
DropdownManyItemsPerf.default 412 382 1.08:1
ItemLayoutMinimalPerf.default 734 689 1.07:1
DialogMinimalPerf.default 457 433 1.06:1
DividerMinimalPerf.default 210 198 1.06:1
ProviderMinimalPerf.default 203 194 1.05:1
StatusMinimalPerf.default 410 389 1.05:1
IconMinimalPerf.default 396 376 1.05:1
TableMinimalPerf.default 239 227 1.05:1
VideoMinimalPerf.default 449 428 1.05:1
ListMinimalPerf.default 317 306 1.04:1
ListNestedPerf.default 334 321 1.04:1
SegmentMinimalPerf.default 196 189 1.04:1
TextAreaMinimalPerf.default 305 293 1.04:1
AvatarMinimalPerf.default 108 105 1.03:1
ButtonOverridesMissPerf.default 653 635 1.03:1
CardMinimalPerf.default 309 300 1.03:1
EmbedMinimalPerf.default 1911 1850 1.03:1
RefMinimalPerf.default 109 106 1.03:1
SliderMinimalPerf.default 745 726 1.03:1
CustomToolbarPrototype.default 1484 1438 1.03:1
BoxMinimalPerf.default 195 192 1.02:1
ButtonMinimalPerf.default 92 90 1.02:1
FormMinimalPerf.default 217 213 1.02:1
GridMinimalPerf.default 190 186 1.02:1
HeaderSlotsPerf.default 470 462 1.02:1
ToolbarMinimalPerf.default 537 525 1.02:1
AnimationMinimalPerf.default 302 300 1.01:1
ChatMinimalPerf.default 443 440 1.01:1
CheckboxMinimalPerf.default 1167 1160 1.01:1
FlexMinimalPerf.default 156 154 1.01:1
LabelMinimalPerf.default 220 217 1.01:1
LayoutMinimalPerf.default 198 196 1.01:1
ListWith60ListItems.default 363 359 1.01:1
ReactionMinimalPerf.default 215 213 1.01:1
SkeletonMinimalPerf.default 193 192 1.01:1
TreeMinimalPerf.default 483 480 1.01:1
ChatWithPopoverPerf.default 204 205 1:1
DatepickerMinimalPerf.default 3602 3608 1:1
DropdownMinimalPerf.default 1410 1412 1:1
ImageMinimalPerf.default 219 220 1:1
MenuMinimalPerf.default 509 510 1:1
MenuButtonMinimalPerf.default 954 955 1:1
RosterPerf.default 1600 1606 1:1
PopupMinimalPerf.default 346 346 1:1
ProviderMergeThemesPerf.default 639 641 1:1
RadioGroupMinimalPerf.default 262 262 1:1
SplitButtonMinimalPerf.default 2249 2244 1:1
AccordionMinimalPerf.default 86 87 0.99:1
ButtonSlotsPerf.default 306 308 0.99:1
CarouselMinimalPerf.default 266 269 0.99:1
ListCommonPerf.default 392 394 0.99:1
TableManyItemsPerf.default 1075 1084 0.99:1
HeaderMinimalPerf.default 198 203 0.98:1
InputMinimalPerf.default 526 539 0.98:1
TreeWith60ListItems.default 86 88 0.98:1
LoaderMinimalPerf.default 191 196 0.97:1
TooltipMinimalPerf.default 1256 1296 0.97:1
AttachmentSlotsPerf.default 606 636 0.95:1
ChatDuplicateMessagesPerf.default 143 153 0.93:1

@fabricteam
Copy link
Collaborator

fabricteam commented Jun 24, 2024

🕵 FluentUIV0 No visual regressions between this PR and main

@fabricteam
Copy link
Collaborator

fabricteam commented Jun 24, 2024

📊 Bundle size report

✅ No changes found

@fabricteam
Copy link
Collaborator

fabricteam commented Jun 24, 2024

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
BaseButton mount 633 627 5000
Breadcrumb mount 1709 1719 1000
Checkbox mount 1728 1767 5000
CheckboxBase mount 1476 1479 5000
ChoiceGroup mount 3110 2952 5000
ComboBox mount 676 687 1000
CommandBar mount 6783 6866 1000
ContextualMenu mount 15799 16000 1000
DefaultButton mount 824 798 5000
DetailsRow mount 2262 2232 5000
DetailsRowFast mount 2236 2256 5000
DetailsRowNoStyles mount 2055 2047 5000
Dialog mount 2799 2836 1000
DocumentCardTitle mount 238 228 1000
Dropdown mount 2017 1989 5000
FocusTrapZone mount 1151 1139 5000
FocusZone mount 1111 1100 5000
GroupedList mount 42502 42540 2
GroupedList virtual-rerender 18230 20369 2
GroupedList virtual-rerender-with-unmount 51693 51884 2
GroupedListV2 mount 241 241 2
GroupedListV2 virtual-rerender 220 215 2
GroupedListV2 virtual-rerender-with-unmount 221 239 2
IconButton mount 1168 1152 5000
Label mount 344 342 5000
Layer mount 2742 2710 5000
Link mount 386 403 5000
MenuButton mount 1014 1036 5000
MessageBar mount 21806 21739 5000
Nav mount 2058 2023 1000
OverflowSet mount 777 833 5000
Panel mount 1942 1808 1000
Persona mount 759 755 1000
Pivot mount 933 911 1000
PrimaryButton mount 962 925 5000
Rating mount 4712 4831 5000
SearchBox mount 931 962 5000
Shimmer mount 1943 1915 5000
Slider mount 1354 1361 5000
SpinButton mount 2977 3015 5000
Spinner mount 392 411 5000
SplitButton mount 1898 1913 5000
Stack mount 437 439 5000
StackWithIntrinsicChildren mount 885 881 5000
StackWithTextChildren mount 2755 2805 5000
SwatchColorPicker mount 6455 6515 5000
TagPicker mount 1459 1458 5000
Text mount 392 393 5000
TextField mount 951 973 5000
ThemeProvider mount 864 875 5000
ThemeProvider virtual-rerender 579 594 5000
ThemeProvider virtual-rerender-with-unmount 1337 1304 5000
Toggle mount 606 641 5000
buttonNative mount 191 187 5000

Copy link
Contributor

@Hotell Hotell left a comment

Choose a reason for hiding this comment

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

npm aliases for lib dom types

npm aliases is a pattern that might not scale pretty well.

from my experience being explicit about global types is only safe bet. with this setup you're basically saying that any exposed global types installed within node_modules/@types will be part of tsc program for web-components.

I'd suggest to keep things as is with explicit types as with this approach TS emit works as expected and provides the extended DOM types as part of your public APIs
image

with the alias approach that would no longer be the case.

Additional thing that needs to be checked is of any of those extended types are part of your public API's. if they are you should be shipping @types/web probably as production dependency or peer dependency requirement otherwise users on older TS might run into build errors.

regarding wc vr test app

pipelines were failing because of incorrectly set globals and @types/web not being installed via SVP ( single version policy approach )

@radium-v radium-v force-pushed the users/radium-v/wc3-web-types branch from 7b4046c to 6e5ee92 Compare June 27, 2024 19:38
@radium-v radium-v requested a review from Hotell June 27, 2024 19:45
"allowJs": true
"allowJs": true,
"lib": ["ESNext", "DOM"],
"types": ["web", "es2023-shim"]
Copy link
Contributor

Choose a reason for hiding this comment

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

TS solution configs shouldn't contain types definition as they make sense only on environment scoped level ( which is .lib.json, '.spec.json', 'storybook', etc ).

also this kind of changes leads to inconsistencies which can be observed in this PR, storybook tsconfig provides types ( this is correct ), while others are not.

please revert this change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this kind of changes leads to inconsistencies which can be observed in this PR, storybook tsconfig provides types ( this is correct ), while others are not.

The types property is inherited from the use of extends. The storybook tsconfig sets its own types property to override the defaults provided here (to add node types). Is there any reason not to use this built-in pattern?

https://www.typescriptlang.org/tsconfig/#extends

The configuration from the base file are loaded first, then overridden by those in the inheriting config file.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand what you wanna do,

Wondering if you read my message ?

TS solution configs shouldn't contain types definition as they make sense only on environment scoped level ( which is .lib.json, '.spec.json', 'storybook', etc ). + following monorepo consistency patterns.

ty

Copy link
Contributor

@Hotell Hotell left a comment

Choose a reason for hiding this comment

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

pls undo those types changes I commented on -> approving to unblock once that is resolved. ty!

@radium-v radium-v force-pushed the users/radium-v/wc3-web-types branch from 6e5ee92 to 4afdb00 Compare June 28, 2024 17:07
@radium-v
Copy link
Contributor Author

radium-v commented Jul 1, 2024

Closing in favor of #31857. The purpose of this PR has shifted too far away from its original intent and isn't necessary at this time.

@radium-v radium-v closed this Jul 1, 2024
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.

None yet

5 participants