Fix minor unaddressed TS 4 upgrade PR comments #18027
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ecraig12345
commented
May 4, 2021
| "@fluentui/web-components/@microsoft/eslint-config-fast-dna", | ||
| "@fluentui/web-components/@storybook/**", | ||
| "@fluentui/web-components/build", |
There was a problem hiding this comment.
This is not a package
| "@fluentui/web-components/ts-loader", | ||
| "@fluentui/web-components/ts-loader/**", | ||
| "@fluentui/web-components/ts-node", | ||
| "@fluentui/web-components/ts-node/**", | ||
| "@fluentui/web-components/typescript", | ||
| "@fluentui/web-components/typescript/**", |
There was a problem hiding this comment.
typescript has no dependencies
| @@ -184,14 +182,6 @@ | |||
| "scripts/package.json" | |||
| ], | |||
| "versionGroups": [ | |||
| { | |||
There was a problem hiding this comment.
This is unneeded because test-bundles no longer uses a different webpack version
5 tasks
Asset size changes
Baseline commit: 59030f591ce877aea56eee6d2b0fb57dec652935 (build) |
|
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 7542dfd:
|
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 940 | 927 | 5000 | |
| BaseButton | mount | 963 | 973 | 5000 | |
| Breadcrumb | mount | 2639 | 2664 | 1000 | |
| ButtonNext | mount | 571 | 583 | 5000 | |
| Checkbox | mount | 1705 | 1639 | 5000 | |
| CheckboxBase | mount | 1398 | 1421 | 5000 | |
| ChoiceGroup | mount | 5066 | 5085 | 5000 | |
| ComboBox | mount | 1012 | 1022 | 1000 | |
| CommandBar | mount | 10399 | 10891 | 1000 | |
| ContextualMenu | mount | 6393 | 6371 | 1000 | |
| DefaultButton | mount | 1222 | 1247 | 5000 | |
| DetailsRow | mount | 3913 | 3960 | 5000 | |
| DetailsRowFast | mount | 3842 | 3865 | 5000 | |
| DetailsRowNoStyles | mount | 3667 | 3666 | 5000 | |
| Dialog | mount | 2259 | 2223 | 1000 | |
| DocumentCardTitle | mount | 152 | 165 | 1000 | |
| Dropdown | mount | 3452 | 3422 | 5000 | |
| FocusTrapZone | mount | 1811 | 1910 | 5000 | |
| FocusZone | mount | 1851 | 1822 | 5000 | |
| IconButton | mount | 1914 | 1890 | 5000 | |
| Label | mount | 337 | 362 | 5000 | |
| Layer | mount | 1905 | 1932 | 5000 | |
| Link | mount | 484 | 489 | 5000 | |
| MakeStyles | mount | 1843 | 1837 | 50000 | |
| MenuButton | mount | 1577 | 1576 | 5000 | |
| MessageBar | mount | 2112 | 2038 | 5000 | |
| Nav | mount | 3467 | 3450 | 1000 | |
| OverflowSet | mount | 1062 | 1060 | 5000 | |
| Panel | mount | 2093 | 2119 | 1000 | |
| Persona | mount | 847 | 874 | 1000 | |
| Pivot | mount | 1508 | 1456 | 1000 | |
| PrimaryButton | mount | 1376 | 1365 | 5000 | |
| Rating | mount | 8190 | 8276 | 5000 | |
| SearchBox | mount | 1415 | 1414 | 5000 | |
| Shimmer | mount | 2726 | 2720 | 5000 | |
| Slider | mount | 2056 | 2109 | 5000 | |
| SpinButton | mount | 5196 | 5176 | 5000 | |
| Spinner | mount | 437 | 431 | 5000 | |
| SplitButton | mount | 3282 | 3327 | 5000 | |
| Stack | mount | 529 | 544 | 5000 | |
| StackWithIntrinsicChildren | mount | 1678 | 1694 | 5000 | |
| StackWithTextChildren | mount | 4976 | 5030 | 5000 | |
| SwatchColorPicker | mount | 10803 | 10608 | 5000 | |
| Tabs | mount | 1453 | 1430 | 1000 | |
| TagPicker | mount | 2551 | 2588 | 5000 | |
| TeachingBubble | mount | 12201 | 12252 | 5000 | |
| Text | mount | 439 | 443 | 5000 | |
| TextField | mount | 1444 | 1501 | 5000 | |
| ThemeProvider | mount | 1192 | 1197 | 5000 | |
| ThemeProvider | virtual-rerender | 584 | 588 | 5000 | |
| ThemeProviderNext | mount | 8699 | 8758 | 5000 | |
| Toggle | mount | 873 | 827 | 5000 | |
| buttonNative | mount | 109 | 109 | 5000 |
Perf Analysis (@fluentui/react-northstar)
Perf tests with no regressions
| Scenario | Current PR Ticks | Baseline Ticks | Ratio |
|---|---|---|---|
| AttachmentMinimalPerf.default | 184 | 167 | 1.1:1 |
| PortalMinimalPerf.default | 176 | 161 | 1.09:1 |
| AvatarMinimalPerf.default | 215 | 200 | 1.08:1 |
| RefMinimalPerf.default | 253 | 235 | 1.08:1 |
| TableMinimalPerf.default | 450 | 418 | 1.08:1 |
| HeaderMinimalPerf.default | 410 | 384 | 1.07:1 |
| LabelMinimalPerf.default | 438 | 409 | 1.07:1 |
| DividerMinimalPerf.default | 401 | 380 | 1.06:1 |
| ButtonMinimalPerf.default | 192 | 183 | 1.05:1 |
| SegmentMinimalPerf.default | 390 | 370 | 1.05:1 |
| ButtonSlotsPerf.default | 636 | 613 | 1.04:1 |
| DropdownManyItemsPerf.default | 758 | 728 | 1.04:1 |
| HeaderSlotsPerf.default | 843 | 808 | 1.04:1 |
| ListWith60ListItems.default | 692 | 667 | 1.04:1 |
| TreeWith60ListItems.default | 192 | 185 | 1.04:1 |
| AnimationMinimalPerf.default | 427 | 415 | 1.03:1 |
| ItemLayoutMinimalPerf.default | 1393 | 1348 | 1.03:1 |
| ReactionMinimalPerf.default | 423 | 412 | 1.03:1 |
| TextAreaMinimalPerf.default | 564 | 550 | 1.03:1 |
| AttachmentSlotsPerf.default | 1253 | 1231 | 1.02:1 |
| ButtonOverridesMissPerf.default | 1787 | 1751 | 1.02:1 |
| ChatWithPopoverPerf.default | 402 | 394 | 1.02:1 |
| EmbedMinimalPerf.default | 4370 | 4294 | 1.02:1 |
| FlexMinimalPerf.default | 313 | 307 | 1.02:1 |
| ImageMinimalPerf.default | 419 | 409 | 1.02:1 |
| LayoutMinimalPerf.default | 396 | 387 | 1.02:1 |
| LoaderMinimalPerf.default | 727 | 710 | 1.02:1 |
| ProviderMinimalPerf.default | 1066 | 1045 | 1.02:1 |
| SkeletonMinimalPerf.default | 393 | 387 | 1.02:1 |
| BoxMinimalPerf.default | 386 | 384 | 1.01:1 |
| CarouselMinimalPerf.default | 497 | 492 | 1.01:1 |
| ChatMinimalPerf.default | 668 | 662 | 1.01:1 |
| DialogMinimalPerf.default | 785 | 781 | 1.01:1 |
| InputMinimalPerf.default | 1315 | 1307 | 1.01:1 |
| PopupMinimalPerf.default | 774 | 763 | 1.01:1 |
| RadioGroupMinimalPerf.default | 482 | 475 | 1.01:1 |
| SliderMinimalPerf.default | 1639 | 1625 | 1.01:1 |
| CustomToolbarPrototype.default | 3953 | 3928 | 1.01:1 |
| ToolbarMinimalPerf.default | 1023 | 1013 | 1.01:1 |
| TreeMinimalPerf.default | 844 | 834 | 1.01:1 |
| CardMinimalPerf.default | 617 | 615 | 1:1 |
| ChatDuplicateMessagesPerf.default | 302 | 303 | 1:1 |
| DatepickerMinimalPerf.default | 5678 | 5699 | 1:1 |
| ListNestedPerf.default | 606 | 607 | 1:1 |
| ProviderMergeThemesPerf.default | 1674 | 1682 | 1:1 |
| SplitButtonMinimalPerf.default | 3985 | 3971 | 1:1 |
| IconMinimalPerf.default | 683 | 683 | 1:1 |
| TooltipMinimalPerf.default | 1024 | 1029 | 1:1 |
| AccordionMinimalPerf.default | 157 | 158 | 0.99:1 |
| ButtonUseCssNestingPerf.default | 1117 | 1124 | 0.99:1 |
| CheckboxMinimalPerf.default | 2863 | 2900 | 0.99:1 |
| DropdownMinimalPerf.default | 3132 | 3169 | 0.99:1 |
| GridMinimalPerf.default | 372 | 376 | 0.99:1 |
| MenuMinimalPerf.default | 916 | 928 | 0.99:1 |
| MenuButtonMinimalPerf.default | 1678 | 1689 | 0.99:1 |
| TableManyItemsPerf.default | 2061 | 2077 | 0.99:1 |
| VideoMinimalPerf.default | 681 | 691 | 0.99:1 |
| FormMinimalPerf.default | 457 | 468 | 0.98:1 |
| TextMinimalPerf.default | 376 | 382 | 0.98:1 |
| ButtonUseCssPerf.default | 857 | 887 | 0.97:1 |
| ListMinimalPerf.default | 542 | 559 | 0.97:1 |
| ListCommonPerf.default | 680 | 711 | 0.96:1 |
| AlertMinimalPerf.default | 287 | 301 | 0.95:1 |
| RosterPerf.default | 1236 | 1317 | 0.94:1 |
| StatusMinimalPerf.default | 747 | 792 | 0.94:1 |
ling1726
reviewed
May 4, 2021
| @@ -122,7 +122,11 @@ async function run() { | |||
|
|
|||
| // do these before deleting node_nodules | |||
| console.log('\nClearing Jest cache...'); | |||
| spawn(os.platform() === 'win32' ? 'npx.cmd' : 'npx', ['jest', '--clearCache']); | |||
| try { | |||
There was a problem hiding this comment.
I have never seen this script being used, can we just remove it ?
There was a problem hiding this comment.
+1, this is a ticking bomb IMHO for anyone not familiar with this monorepo setup - removing untracked files etc should be done manually by git by current user.
There was a problem hiding this comment.
lets discuss this during @microsoft/fluentui-react-build sync
There was a problem hiding this comment.
This is never run automatically, and it explicitly requires confirmation when you start the script. It's intended to help recover the repo from a bad state, in a way compatible with Windows (IIRC git clean -fdx sometimes incorrectly handles symlinks on Windows), and that handles a couple caches that are outside the repo. In my experience this can be quite important at times, and not a "time bomb" for the reasons mentioned above.
ling1726
approved these changes
May 4, 2021
layershifter
approved these changes
May 4, 2021
khmakoto
approved these changes
May 4, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull request checklist
$ yarn changeDescription of changes
Fix some minor unaddressed comments on #17932:
nohoistandversionGroupssettingsAlso made a minor fix to the
scrubscript to make it work with outdated repo builds (when I started on this branch's changes in an old worktree, the script failed because jest couldn't find a build of some newer local helper).