-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Conversation
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:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: f7d7f82f6b3117ce163c6fee84b03c8774bed95f (build) |
Perf Analysis (
|
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 |
📊 Bundle size reportUnchanged fixtures
|
🕵 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
|
Perf Analysis (
|
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 |
🕵 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
|
c9d949b
to
3b68a5d
Compare
"preserveConstEnums": true, | ||
"strictNullChecks": true, | ||
"noImplicitAny": true, | ||
"lib": ["es6", "dom"], | ||
"types": ["webpack-env"], | ||
"skipLibCheck": true | ||
"types": ["webpack-env", "node"] |
There was a problem hiding this comment.
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)
Perf Analysis (
|
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"], |
There was a problem hiding this comment.
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
6fbe5a0
to
24b62a2
Compare
24b62a2
to
9c28e17
Compare
"preserveConstEnums": true, | ||
"noImplicitThis": true, |
There was a problem hiding this comment.
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.
…ing ts path aliases
…align with v9 packages and to leverage path aliases
8cece11
to
5dde7c6
Compare
@@ -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 } = { |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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 => { |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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 = '.'; |
There was a problem hiding this comment.
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": [ |
There was a problem hiding this comment.
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.
* 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)
* 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
Previous Behavior
dev workflow when working on any v8 + v8 related package is following:
New Behavior
this PR introduces DX as we have in v9 on following scope
apps/
"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)