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: introduce TS path aliases for improved DX in v8 #25778

Merged
merged 9 commits into from
Nov 30, 2022

Conversation

Hotell
Copy link
Contributor

@Hotell Hotell commented Nov 24, 2022

Previous Behavior

dev workflow when working on any v8 + v8 related package is following:

  • need to build whole dependency chain to get proper TS autocomplete and errors
  • need to re-build while dependency chain on any change cross package
  • refactoring cross package doesn't work

New Behavior

this PR introduces DX as we have in v9 on following scope

  • root tsconfig base with path aliases for all v8 and v8 related packages
  • applying path aliases in v8 apps/
  • fixing all v8 apps to be compliant with "strictNullChecks": true and "noImplicitAny": true,

NOTE:

This is a first step in migrating all v8 to v9 like DX.
This PR will not change/improve performance of apps builds by leveraging path aliases (this will be done as follow-up), it only enables TypeScript intelisense and refactorings within all v8 apps/ *

Related Issue(s)

@Hotell Hotell changed the title Hotell/v8/path aliases chore: introduce TS path aliases for v8 improved DX Nov 24, 2022
@Hotell Hotell removed this from the October Project Cycle Q4 2022 milestone Nov 24, 2022
@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 24, 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 3427d4f:

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

@size-auditor
Copy link

size-auditor bot commented Nov 24, 2022

Asset size changes

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

Baseline commit: f7d7f82f6b3117ce163c6fee84b03c8774bed95f (build)

@fabricteam
Copy link
Collaborator

fabricteam commented Nov 24, 2022

Perf Analysis (@fluentui/react-components)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 1569 1572 5000
Button mount 1105 1134 5000
FluentProvider mount 1875 1895 5000
FluentProviderWithTheme mount 739 771 10
FluentProviderWithTheme virtual-rerender 709 700 10
FluentProviderWithTheme virtual-rerender-with-unmount 739 764 10
MakeStyles mount 2274 2318 50000
SpinButton mount 3040 2999 5000

@fabricteam
Copy link
Collaborator

fabricteam commented Nov 24, 2022

📊 Bundle size report

Unchanged fixtures
Package & Exports Size (minified/GZIP)
global-context
createContext
533 B
341 B
global-context
createContextSelector
554 B
348 B
priority-overflow
createOverflowManager
3.153 kB
1.299 kB
react-accordion
Accordion (including children components)
79.294 kB
24.443 kB
react-alert
Alert
86.891 kB
21.708 kB
react-avatar
Avatar
51.076 kB
14.285 kB
react-avatar
AvatarGroup
15.006 kB
6.009 kB
react-avatar
AvatarGroupItem
65.654 kB
18.382 kB
react-badge
Badge
23.357 kB
7.361 kB
react-badge
CounterBadge
24.247 kB
7.643 kB
react-badge
PresenceBadge
24.001 kB
7.049 kB
react-button
Button
36.86 kB
9.913 kB
react-button
CompoundButton
43.885 kB
11.131 kB
react-button
MenuButton
41.537 kB
11.187 kB
react-button
SplitButton
48.983 kB
12.588 kB
react-button
ToggleButton
52.685 kB
11.415 kB
react-card
Card - All
71.873 kB
20.936 kB
react-card
Card
67.651 kB
19.892 kB
react-card
CardFooter
8.617 kB
3.62 kB
react-card
CardHeader
9.792 kB
3.995 kB
react-card
CardPreview
8.718 kB
3.674 kB
react-combobox
Combobox (including child components)
78.244 kB
25.015 kB
react-combobox
Dropdown (including child components)
77.531 kB
24.927 kB
react-components
react-components: Button, FluentProvider & webLightTheme
62.975 kB
17.687 kB
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
190.411 kB
53.077 kB
react-components
react-components: FluentProvider & webLightTheme
33.48 kB
11.037 kB
react-dialog
Dialog (including children components)
83.036 kB
24.754 kB
react-divider
Divider
16.515 kB
5.921 kB
react-image
Image
10.836 kB
4.283 kB
react-infobutton
InfoButton
117.922 kB
35.391 kB
react-input
Input
24.194 kB
7.89 kB
react-label
Label
9.394 kB
3.88 kB
react-link
Link
11.862 kB
4.885 kB
react-menu
Menu (including children components)
117.61 kB
36.287 kB
react-menu
Menu (including selectable components)
120.679 kB
36.82 kB
react-overflow
hooks only
11.004 kB
4.188 kB
react-persona
Persona
57.131 kB
15.951 kB
react-popover
Popover
103.666 kB
31.858 kB
react-portal
Portal
10.495 kB
3.851 kB
react-portal-compat
PortalCompatProvider
5.857 kB
1.978 kB
react-positioning
usePositioning
19.826 kB
7.417 kB
react-progress
Progress
13.536 kB
5.107 kB
react-provider
FluentProvider
15.817 kB
5.905 kB
react-radio
Radio
36.446 kB
12.123 kB
react-radio
RadioGroup
14.304 kB
5.72 kB
react-select
Select
23.474 kB
8.017 kB
react-slider
Slider
32.118 kB
10.192 kB
react-spinbutton
SpinButton
44.502 kB
12.812 kB
react-spinner
Spinner
20.013 kB
6.446 kB
react-switch
Switch
33.453 kB
10.581 kB
react-text
Text - Default
11.838 kB
4.625 kB
react-text
Text - Wrappers
15.148 kB
5.06 kB
react-textarea
Textarea
25.733 kB
8.328 kB
react-theme
Single theme token import
69 B
89 B
react-theme
Teams: all themes
29.722 kB
6.462 kB
react-theme
Teams: Light theme
17.51 kB
5.075 kB
react-tooltip
Tooltip
42.032 kB
14.739 kB
react-utilities
SSRProvider
180 B
159 B
🤖 This report was generated against f7d7f82f6b3117ce163c6fee84b03c8774bed95f

@fabricteam
Copy link
Collaborator

fabricteam commented Nov 24, 2022

🕵 fluentuiv8 Open the Visual Regressions report to inspect the 5 screenshots

✅ There was 2 screenshots added, 0 screenshots removed, 1034 screenshots unchanged, 0 screenshots with different dimensions and 3 screenshots with visible difference.

unknown 5 screenshots
Image Name Diff(in Pixels) Image Type
TagPicker.Root.chromium.png 160 Changed
TagPicker.Selected - RTL.chromium.png 160 Changed
TagPicker.Selected.chromium.png 160 Changed
Pivot - Overflow.Root.Narrow - Last tab selected.chromium.png 0 Added
Pivot - Overflow.Root.Narrow - Overflow menu.chromium.png 0 Added

@fabricteam
Copy link
Collaborator

fabricteam commented Nov 24, 2022

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
AvatarMinimalPerf.default 190 168 1.13:1
DividerMinimalPerf.default 350 316 1.11:1
RefMinimalPerf.default 209 193 1.08:1
LabelMinimalPerf.default 357 342 1.04:1
ListNestedPerf.default 528 509 1.04:1
CustomToolbarPrototype.default 2627 2529 1.04:1
TreeWith60ListItems.default 154 148 1.04:1
AttachmentSlotsPerf.default 1054 1025 1.03:1
ImageMinimalPerf.default 364 353 1.03:1
ListCommonPerf.default 592 577 1.03:1
RosterPerf.default 2080 2025 1.03:1
PortalMinimalPerf.default 156 152 1.03:1
ProviderMinimalPerf.default 389 376 1.03:1
TextAreaMinimalPerf.default 450 435 1.03:1
VideoMinimalPerf.default 687 666 1.03:1
CarouselMinimalPerf.default 436 428 1.02:1
ChatDuplicateMessagesPerf.default 259 255 1.02:1
CheckboxMinimalPerf.default 1982 1949 1.02:1
GridMinimalPerf.default 307 301 1.02:1
HeaderMinimalPerf.default 332 326 1.02:1
LoaderMinimalPerf.default 306 301 1.02:1
MenuMinimalPerf.default 807 788 1.02:1
SkeletonMinimalPerf.default 322 316 1.02:1
SplitButtonMinimalPerf.default 4186 4119 1.02:1
AlertMinimalPerf.default 254 251 1.01:1
AnimationMinimalPerf.default 486 481 1.01:1
ButtonOverridesMissPerf.default 1230 1221 1.01:1
CardMinimalPerf.default 501 494 1.01:1
ChatWithPopoverPerf.default 342 340 1.01:1
DropdownMinimalPerf.default 2567 2546 1.01:1
FormMinimalPerf.default 359 355 1.01:1
InputMinimalPerf.default 1078 1071 1.01:1
MenuButtonMinimalPerf.default 1621 1601 1.01:1
SegmentMinimalPerf.default 320 316 1.01:1
SliderMinimalPerf.default 1528 1507 1.01:1
TableMinimalPerf.default 374 370 1.01:1
TextMinimalPerf.default 314 311 1.01:1
TreeMinimalPerf.default 756 750 1.01:1
DatepickerMinimalPerf.default 5408 5385 1:1
DropdownManyItemsPerf.default 636 634 1:1
EmbedMinimalPerf.default 3491 3489 1:1
ProviderMergeThemesPerf.default 1227 1230 1:1
RadioGroupMinimalPerf.default 408 407 1:1
StatusMinimalPerf.default 644 646 1:1
TableManyItemsPerf.default 1770 1768 1:1
ToolbarMinimalPerf.default 864 868 1:1
TooltipMinimalPerf.default 2180 2188 1:1
AttachmentMinimalPerf.default 141 143 0.99:1
BoxMinimalPerf.default 306 308 0.99:1
ButtonSlotsPerf.default 508 514 0.99:1
DialogMinimalPerf.default 730 736 0.99:1
HeaderSlotsPerf.default 718 726 0.99:1
ItemLayoutMinimalPerf.default 1118 1126 0.99:1
LayoutMinimalPerf.default 330 332 0.99:1
ListMinimalPerf.default 474 480 0.99:1
PopupMinimalPerf.default 587 591 0.99:1
AccordionMinimalPerf.default 133 136 0.98:1
ChatMinimalPerf.default 665 678 0.98:1
FlexMinimalPerf.default 261 265 0.98:1
IconMinimalPerf.default 592 605 0.98:1
ListWith60ListItems.default 571 586 0.97:1
ReactionMinimalPerf.default 343 358 0.96:1
ButtonMinimalPerf.default 144 153 0.94:1

@fabricteam
Copy link
Collaborator

fabricteam commented Nov 24, 2022

🕵 fluentuiv9 Open the Visual Regressions report to inspect the 2 screenshots

✅ There was 0 screenshots added, 0 screenshots removed, 1749 screenshots unchanged, 0 screenshots with different dimensions and 2 screenshots with visible difference.

unknown 2 screenshots
Image Name Diff(in Pixels) Image Type
Avatar Converged.size+active+badge.normal.chromium.png 8 Changed
Avatar Converged.size+active+ring-shadow.normal.chromium.png 7 Changed

@Hotell Hotell force-pushed the hotell/v8/path-aliases branch from c9d949b to 3b68a5d Compare November 24, 2022 16:33
"preserveConstEnums": true,
"strictNullChecks": true,
"noImplicitAny": true,
"lib": ["es6", "dom"],
"types": ["webpack-env"],
"skipLibCheck": true
"types": ["webpack-env", "node"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

node is needed because some v8 packages are relying on that global type existence (they are using NodeJS.* namespace which is exposed on global if node types is present)

@fabricteam
Copy link
Collaborator

fabricteam commented Nov 24, 2022

Perf Analysis (@fluentui/react)

Scenario Render type Master Ticks PR Ticks Iterations Status
GroupedList mount 1804 2211 2 Possible regression
All results

Scenario Render type Master Ticks PR Ticks Iterations Status
BaseButton mount 1192 1206 5000
Breadcrumb mount 2788 2783 1000
Checkbox mount 2629 2623 5000
CheckboxBase mount 2344 2388 5000
ChoiceGroup mount 4285 4296 5000
ComboBox mount 1171 1171 1000
CommandBar mount 9248 9250 1000
ContextualMenu mount 10064 10056 1000
DefaultButton mount 1371 1359 5000
DetailsRow mount 3381 3348 5000
DetailsRowFast mount 3400 3395 5000
DetailsRowNoStyles mount 3234 3203 5000
Dialog mount 2949 2944 1000
DocumentCardTitle mount 584 570 1000
Dropdown mount 3152 3168 5000
FocusTrapZone mount 1945 1974 5000
FocusZone mount 1871 1910 5000
GroupedList mount 1804 2211 2 Possible regression
GroupedList virtual-rerender 1080 1100 2
GroupedList virtual-rerender-with-unmount 1594 1619 2
GroupedListV2 mount 561 559 2
GroupedListV2 virtual-rerender 545 544 2
GroupedListV2 virtual-rerender-with-unmount 566 565 2
IconButton mount 1775 1779 5000
Label mount 747 756 5000
Layer mount 4192 4180 5000
Link mount 839 854 5000
MenuButton mount 1622 1603 5000
MessageBar mount 2350 2342 5000
Nav mount 3078 3052 1000
OverflowSet mount 1404 1415 5000
Panel mount 2495 2460 1000
Persona mount 1255 1296 1000
Pivot mount 1520 1535 1000
PrimaryButton mount 1488 1475 5000
Rating mount 6973 6992 5000
SearchBox mount 1520 1485 5000
Shimmer mount 2914 2913 5000
Slider mount 2113 2074 5000
SpinButton mount 4268 4214 5000
Spinner mount 820 824 5000
SplitButton mount 2873 2842 5000
Stack mount 872 875 5000
StackWithIntrinsicChildren mount 2327 2324 5000
StackWithTextChildren mount 5054 5018 5000
SwatchColorPicker mount 9432 9451 5000
TagPicker mount 2363 2348 5000
TeachingBubble mount 74032 73308 5000
Text mount 830 826 5000
TextField mount 1527 1530 5000
ThemeProvider mount 1440 1436 5000
ThemeProvider virtual-rerender 1143 1144 5000
ThemeProvider virtual-rerender-with-unmount 1995 1984 5000
Toggle mount 1113 1125 5000
buttonNative mount 537 531 5000

"rootDir": ".",
"baseUrl": ".",
"paths": {
"@fluentui/api-docs": ["packages/api-docs/src/index.ts"],
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 map has been verified on whole monorepo in this PR, https://github.com/microsoft/fluentui/pull/22604/files

@Hotell Hotell removed this from the October Project Cycle Q4 2022 milestone Nov 25, 2022
@Hotell Hotell changed the title chore: introduce TS path aliases for v8 improved DX chore: introduce TS path aliases for improved DX in v8 Nov 25, 2022
@Hotell Hotell force-pushed the hotell/v8/path-aliases branch from 6fbe5a0 to 24b62a2 Compare November 25, 2022 12:10
@Hotell Hotell force-pushed the hotell/v8/path-aliases branch from 24b62a2 to 9c28e17 Compare November 25, 2022 12:10
"preserveConstEnums": true,
"noImplicitThis": true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

v8 will not compile with noImplicitThis so removing from config.

Note that no this is used within this codebase.

All packages needs to conform to same TS compiler standards. One of many advantages using path aliases for robust and uniform code.

@Hotell Hotell force-pushed the hotell/v8/path-aliases branch from 8cece11 to 5dde7c6 Compare November 25, 2022 18:52
@@ -12,7 +12,7 @@ export interface ICategory {
// Exporting this object to be used in generating a TOC (table of content) for docs.microsoft documentation repo.
// Any changes to this object need to be communicated to avoid accidental breaking of the documentation
// and to allow the appropriate actions to be taken to mitigate this.
export const categories: { Other?: ICategory; [name: string]: ICategory } = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

invalid type exposed by no strict null checks

@@ -96,7 +96,7 @@ export class Site<TPlatforms extends string = string> extends React.Component<
let platform = 'default' as TPlatforms;

// If current page doesn't have pages for the active platform, switch to its first platform.
if (Object.keys(navData.pagePlatforms).length > 0 && navData.activePages.length === 0) {
if (Object.keys(navData.pagePlatforms!).length > 0 && navData.activePages!.length === 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

all these nullable issues were exposed by no strict null checks, while using ! is deprecated and instead ? should be used, to be able to not break anything website related, this is the safest approach.

@@ -70,15 +70,15 @@ export class NotFoundPage extends React.Component<INotFoundPageProps, {}> {
];

/** Gets the top level page from the current URL and returns a link to it. */
private _getAreaLink = (): JSX.Element => {
private _getAreaLink = (): JSX.Element | undefined => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

lot of this pattern around the website codebase. function that returns nothing but is annotated as return always a type


_otherControlsRequestSections(platform) !== undefined && sections.push(_otherControlsRequestSections(platform));
const _otherControlsRequestSectionsValue = _otherControlsRequestSections(platform);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

typescript cannot narrow down nullables via shortcircuiting - separate assignement needs to be created and used in conditions.

@@ -219,7 +219,7 @@ export function setIconOptions(options: Partial<IIconOptions>): void {
}

let _missingIcons: string[] = [];
let _missingIconsTimer: number | undefined = undefined;
let _missingIconsTimer: ReturnType<typeof setTimeout> | undefined = undefined;
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 wont be number in node js, thus this is the safe approach to have uniform data types across environments.

  • in browser it will be number
  • in nodejs NodeJS.Time


if (isUsingV8pathAliases) {
logger.info(`📣 TSC: V8 package is using TS path aliases. Overriding tsconfig settings.`);
options.baseUrl = '.';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

turning path aliases off for actual production compilation to not break existing flows

"@fluentui/public-docsite-setup": ["packages/public-docsite-setup/src/index.ts"],
"@fluentui/react-docsite-components": ["packages/react-docsite-components/src/index.ts"],
"@fluentui/react-docsite-components/lib/index2": ["packages/react-docsite-components/src/index2.ts"],
"@fluentui/monaco-editor": [
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 is quite tricky as monaco editor definitions are created on build, for sake of intellisense this is all that's needed, but if it would be used for build the monaco-editor build would need to be trigger in advance. thankfully only public-docsite v8 uses this editor in whole monorepo so everything else is all set.

@Hotell Hotell marked this pull request as ready for review November 28, 2022 11:17
@Hotell Hotell requested review from dzearing and a team as code owners November 28, 2022 11:17
@Hotell Hotell removed this from the October Project Cycle Q4 2022 milestone Nov 28, 2022
@Hotell Hotell merged commit 5079ab5 into microsoft:master Nov 30, 2022
@Hotell Hotell deleted the hotell/v8/path-aliases branch November 30, 2022 16:52
marcosmoura added a commit to marcosmoura/fluentui that referenced this pull request Dec 1, 2022
* master:
  BREAKING: `useTable` renamed to `useTableFeatures` (microsoft#25797)
  chore: add retries for navigation in ssr-tests-v9 (microsoft#25844)
  fix: Cell actions should have correct background when row focused within (microsoft#25790)
  applying package updates
  Disable 3 Avatar Converged active stories (microsoft#25765)
  chore: introduce TS path aliases for improved DX in v8 (microsoft#25778)
  chore: prepare release react-northstar 0.65.0 (microsoft#25446)
  refactor(scripts): encapsulate v0 and v8 tooling within its domain boundaries (microsoft#25738)
  Support single point in area chart (microsoft#25842)
  chore: enable isolateModules in all v8 packages (microsoft#25774)
  chore: refactor styles for Button (microsoft#25216)
  feat: Improve docs for `DataGrid`, export as unstable (microsoft#25805)
  applying package updates
  fix: Allow data-selection-disabled to be respected by DetailsRow (microsoft#25836)
  docs(rfcs): Update recipes rfc with chosen option and add more details (microsoft#25823)
  chore(react-textarea): migrate to new package structure (microsoft#25820)
  chore(react-switch): migrate to new package structure (microsoft#25819)
  fix(react-avatar): AvatarGroup's pie layout places inline items correctly in rtl (microsoft#25822)
  chore: add few improvements to toolbar stories (microsoft#25635)
Hotell added a commit to Hotell/fluentui that referenced this pull request Feb 9, 2023
* chore: add v8 base tsconfig with relevant path aliases

* chore: use v8 path aliases for v8 related apps/*

* chore(react-18-tests-v8): follow same pattern with type-check as in v9 when using path aliases

* chore(merge-styles): resolve leaking global type issues exposed by using ts path aliases

* generate changefiles

* fix(theming-designer): fix ts errors exposed by common settings in base.v8

* chore(public-docsite): enable strictNullChecks and noImplicit any to align with v9 packages and to leverage path aliases

* feat(scripts): add  v8 package compilation if TS path aliases are used
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants