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: move normalize-import to scripts so we can reuse it for new dx migration tool #18401

Conversation

Hotell
Copy link
Contributor

@Hotell Hotell commented Jun 1, 2021

Pull request checklist

Description of changes

  • prerequisite to our migration tooling for new DX

Focus areas to test

(optional)

@Hotell Hotell changed the title Hotell/build system/move normalize imports to scripts chore: move normalize-import to scripts so we can reuse it for new dx migration tool Jun 1, 2021
@@ -67,7 +67,7 @@ export const useOnClickOutside = (options: UseOnClickOrScrollOutsideOptions) =>
}

// Garbage collect this event after it's no longer useful to avoid memory leaks
timeoutId.current = setTimeout(() => {
timeoutId.current = window.setTimeout(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

without being explicit regarding from what global context are we accessing props TS will fail (basically local api-extractor build for react-menu was broken because of this)

Copy link
Member

Choose a reason for hiding this comment

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

please use the canUseDom utility, otherwise this will crash during SSR.

cc @layershifter

Copy link
Contributor Author

@Hotell Hotell Jun 1, 2021

Choose a reason for hiding this comment

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

do we need to use that in this PR ? there are multiple other occurrences in other projects which are using window

canUseDom

where does it live ?

Copy link
Member

Choose a reason for hiding this comment

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

kudos to @layershifter for looking into this specifically and finding out that at least for Next.js window and document usages seem to not throw (??!!??)

It might still be better to be explicit about the SSR case. the utility function I mentioned in the same package:

https://github.com/microsoft/fluentui/blob/master/packages/react-utilities/src/ssr/canUseDOM.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.

I just realised, all of this is happening within useEffect which AFAIR is not executed when SSR, thus I'd leave it as is and do the necessary changes in other PR. there are other uses of window directly within this file.

2021-06-02 at 12 31

Are we ok with this ? thx

@size-auditor
Copy link

size-auditor bot commented Jun 1, 2021

Asset size changes

Project Bundle Baseline Size New Size Difference
office-ui-fabric-react fluentui-react-components-Menu 101.919 kB 101.926 kB ExceedsBaseline     7 bytes

ExceedsTolerance Over Tolerance (1024 B) ExceedsBaseline Over Baseline BelowBaseline Below Baseline New New Deleted  Removed 1 kB = 1000 B

Baseline commit: cdc8b4768ede2349bee2c97b6c2860794ec075d1 (build)

@fabricteam
Copy link
Collaborator

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 872 835 5000
BaseButton mount 973 959 5000
Breadcrumb mount 2672 2623 1000
ButtonNext mount 517 511 5000
Checkbox mount 1621 1590 5000
CheckboxBase mount 1354 1374 5000
ChoiceGroup mount 5067 5062 5000
ComboBox mount 1000 1038 1000
CommandBar mount 10213 10185 1000
ContextualMenu mount 6142 6225 1000
DefaultButton mount 1211 1191 5000
DetailsRow mount 3976 3797 5000
DetailsRowFast mount 3784 3942 5000
DetailsRowNoStyles mount 3644 3632 5000
Dialog mount 2204 2172 1000
DocumentCardTitle mount 158 151 1000
Dropdown mount 3444 3499 5000
FocusTrapZone mount 1835 1807 5000
FocusZone mount 1814 1840 5000
IconButton mount 1860 1829 5000
Label mount 345 344 5000
Layer mount 1871 1876 5000
Link mount 489 480 5000
MakeStyles mount 1757 1759 50000
MenuButton mount 1559 1555 5000
MessageBar mount 2078 2089 5000
Nav mount 3411 3448 1000
OverflowSet mount 1031 1125 5000
Panel mount 2105 2059 1000
Persona mount 837 835 1000
Pivot mount 1438 1448 1000
PrimaryButton mount 1337 1356 5000
Rating mount 8274 8211 5000
SearchBox mount 1407 1402 5000
Shimmer mount 2847 2660 5000
Slider mount 2141 2187 5000
SpinButton mount 5295 5360 5000
Spinner mount 417 401 5000
SplitButton mount 3346 3323 5000
Stack mount 554 596 5000
StackWithIntrinsicChildren mount 1679 1675 5000
StackWithTextChildren mount 5095 4999 5000
SwatchColorPicker mount 10788 10893 5000
Tabs mount 1466 1497 1000
TagPicker mount 2610 2650 5000
TeachingBubble mount 12104 12242 5000
Text mount 448 454 5000
TextField mount 1495 1448 5000
ThemeProvider mount 1235 1221 5000
ThemeProvider virtual-rerender 617 622 5000
ThemeProviderNext mount 6558 6594 5000
Toggle mount 877 821 5000
buttonNative mount 124 112 5000

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
AccordionMinimalPerf.default 182 147 1.24:1
AttachmentMinimalPerf.default 190 156 1.22:1
FormMinimalPerf.default 492 423 1.16:1
ChatWithPopoverPerf.default 413 369 1.12:1
TreeWith60ListItems.default 201 181 1.11:1
SkeletonMinimalPerf.default 403 370 1.09:1
ImageMinimalPerf.default 424 391 1.08:1
RefMinimalPerf.default 246 228 1.08:1
ButtonMinimalPerf.default 195 182 1.07:1
GridMinimalPerf.default 367 343 1.07:1
ListCommonPerf.default 693 645 1.07:1
AvatarMinimalPerf.default 224 211 1.06:1
AlertMinimalPerf.default 313 299 1.05:1
AnimationMinimalPerf.default 465 443 1.05:1
MenuButtonMinimalPerf.default 1749 1666 1.05:1
TableMinimalPerf.default 460 439 1.05:1
VideoMinimalPerf.default 715 680 1.05:1
ListNestedPerf.default 622 599 1.04:1
TableManyItemsPerf.default 2163 2077 1.04:1
TextAreaMinimalPerf.default 606 583 1.04:1
ButtonOverridesMissPerf.default 1802 1749 1.03:1
ButtonSlotsPerf.default 599 581 1.03:1
HeaderMinimalPerf.default 400 387 1.03:1
HeaderSlotsPerf.default 830 809 1.03:1
MenuMinimalPerf.default 915 892 1.03:1
IconMinimalPerf.default 683 662 1.03:1
DropdownManyItemsPerf.default 770 756 1.02:1
DropdownMinimalPerf.default 3184 3127 1.02:1
InputMinimalPerf.default 1325 1299 1.02:1
LayoutMinimalPerf.default 391 382 1.02:1
ProviderMergeThemesPerf.default 1687 1659 1.02:1
CustomToolbarPrototype.default 4044 3966 1.02:1
AttachmentSlotsPerf.default 1227 1217 1.01:1
BoxMinimalPerf.default 370 365 1.01:1
CarouselMinimalPerf.default 489 483 1.01:1
ChatMinimalPerf.default 657 648 1.01:1
CheckboxMinimalPerf.default 2810 2778 1.01:1
EmbedMinimalPerf.default 4319 4281 1.01:1
FlexMinimalPerf.default 300 297 1.01:1
ItemLayoutMinimalPerf.default 1362 1346 1.01:1
SplitButtonMinimalPerf.default 4086 4026 1.01:1
TooltipMinimalPerf.default 1055 1040 1.01:1
ListWith60ListItems.default 682 679 1:1
ReactionMinimalPerf.default 420 420 1:1
SegmentMinimalPerf.default 386 385 1:1
SliderMinimalPerf.default 1606 1610 1:1
ToolbarMinimalPerf.default 1019 1014 1:1
DatepickerMinimalPerf.default 5548 5604 0.99:1
RadioGroupMinimalPerf.default 477 480 0.99:1
ChatDuplicateMessagesPerf.default 291 296 0.98:1
DialogMinimalPerf.default 796 815 0.98:1
LabelMinimalPerf.default 403 412 0.98:1
ListMinimalPerf.default 544 554 0.98:1
StatusMinimalPerf.default 734 746 0.98:1
CardMinimalPerf.default 617 639 0.97:1
PopupMinimalPerf.default 580 596 0.97:1
PortalMinimalPerf.default 190 195 0.97:1
ProviderMinimalPerf.default 1041 1075 0.97:1
TreeMinimalPerf.default 846 869 0.97:1
RosterPerf.default 1240 1302 0.95:1
TextMinimalPerf.default 385 406 0.95:1
LoaderMinimalPerf.default 720 773 0.93:1
DividerMinimalPerf.default 386 424 0.91:1

@Hotell Hotell marked this pull request as ready for review June 1, 2021 11:49
@Hotell Hotell requested a review from a team as a code owner June 1, 2021 11:49
@Hotell Hotell merged commit 01f798f into microsoft:master Jun 2, 2021
@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants