-
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
Add conformance test for event callback typing: consistent-callback-type
#30200
Conversation
📊 Bundle size reportUnchanged fixtures
|
Perf Analysis (
|
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
FluentProviderWithTheme | virtual-rerender-with-unmount | 79 | 72 | 10 | Possible regression |
All results
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
Avatar | mount | 639 | 614 | 5000 | |
Button | mount | 293 | 296 | 5000 | |
Field | mount | 1137 | 1129 | 5000 | |
FluentProvider | mount | 704 | 717 | 5000 | |
FluentProviderWithTheme | mount | 77 | 89 | 10 | |
FluentProviderWithTheme | virtual-rerender | 62 | 60 | 10 | |
FluentProviderWithTheme | virtual-rerender-with-unmount | 79 | 72 | 10 | Possible regression |
MakeStyles | mount | 879 | 902 | 50000 | |
Persona | mount | 1796 | 1755 | 5000 | |
SpinButton | mount | 1364 | 1377 | 5000 |
Perf Analysis (
|
Scenario | Current PR Ticks | Baseline Ticks | Ratio |
---|---|---|---|
AvatarMinimalPerf.default | 117 | 102 | 1.15:1 |
ChatDuplicateMessagesPerf.default | 155 | 138 | 1.12:1 |
ButtonMinimalPerf.default | 92 | 83 | 1.11:1 |
AccordionMinimalPerf.default | 81 | 75 | 1.08:1 |
AlertMinimalPerf.default | 159 | 148 | 1.07:1 |
CarouselMinimalPerf.default | 265 | 249 | 1.06:1 |
RefMinimalPerf.default | 112 | 106 | 1.06:1 |
VideoMinimalPerf.default | 442 | 418 | 1.06:1 |
AttachmentMinimalPerf.default | 85 | 81 | 1.05:1 |
ChatMinimalPerf.default | 435 | 416 | 1.05:1 |
ImageMinimalPerf.default | 229 | 219 | 1.05:1 |
ToolbarMinimalPerf.default | 540 | 512 | 1.05:1 |
TreeWith60ListItems.default | 82 | 78 | 1.05:1 |
ButtonSlotsPerf.default | 324 | 313 | 1.04:1 |
DropdownManyItemsPerf.default | 379 | 364 | 1.04:1 |
PopupMinimalPerf.default | 353 | 341 | 1.04:1 |
ProviderMergeThemesPerf.default | 650 | 624 | 1.04:1 |
TooltipMinimalPerf.default | 1301 | 1245 | 1.04:1 |
AnimationMinimalPerf.default | 292 | 283 | 1.03:1 |
HeaderSlotsPerf.default | 475 | 461 | 1.03:1 |
MenuButtonMinimalPerf.default | 952 | 920 | 1.03:1 |
TextAreaMinimalPerf.default | 290 | 282 | 1.03:1 |
CheckboxMinimalPerf.default | 1120 | 1097 | 1.02:1 |
LayoutMinimalPerf.default | 205 | 201 | 1.02:1 |
MenuMinimalPerf.default | 497 | 485 | 1.02:1 |
PortalMinimalPerf.default | 87 | 85 | 1.02:1 |
SliderMinimalPerf.default | 734 | 723 | 1.02:1 |
TextMinimalPerf.default | 193 | 190 | 1.02:1 |
CardMinimalPerf.default | 311 | 309 | 1.01:1 |
LoaderMinimalPerf.default | 191 | 190 | 1.01:1 |
RosterPerf.default | 1569 | 1553 | 1.01:1 |
SegmentMinimalPerf.default | 188 | 186 | 1.01:1 |
SplitButtonMinimalPerf.default | 2215 | 2200 | 1.01:1 |
StatusMinimalPerf.default | 396 | 391 | 1.01:1 |
TableManyItemsPerf.default | 1112 | 1101 | 1.01:1 |
TreeMinimalPerf.default | 473 | 470 | 1.01:1 |
ChatWithPopoverPerf.default | 191 | 191 | 1:1 |
EmbedMinimalPerf.default | 1832 | 1823 | 1:1 |
InputMinimalPerf.default | 539 | 537 | 1:1 |
ListMinimalPerf.default | 306 | 307 | 1:1 |
ListNestedPerf.default | 320 | 319 | 1:1 |
SkeletonMinimalPerf.default | 196 | 196 | 1:1 |
AttachmentSlotsPerf.default | 635 | 643 | 0.99:1 |
BoxMinimalPerf.default | 186 | 188 | 0.99:1 |
DatepickerMinimalPerf.default | 3431 | 3475 | 0.99:1 |
DialogMinimalPerf.default | 434 | 439 | 0.99:1 |
DropdownMinimalPerf.default | 1422 | 1433 | 0.99:1 |
ItemLayoutMinimalPerf.default | 683 | 691 | 0.99:1 |
TableMinimalPerf.default | 228 | 231 | 0.99:1 |
CustomToolbarPrototype.default | 1441 | 1458 | 0.99:1 |
GridMinimalPerf.default | 187 | 191 | 0.98:1 |
RadioGroupMinimalPerf.default | 257 | 262 | 0.98:1 |
DividerMinimalPerf.default | 201 | 208 | 0.97:1 |
FlexMinimalPerf.default | 154 | 158 | 0.97:1 |
ListCommonPerf.default | 388 | 400 | 0.97:1 |
ProviderMinimalPerf.default | 193 | 199 | 0.97:1 |
IconMinimalPerf.default | 377 | 387 | 0.97:1 |
ButtonOverridesMissPerf.default | 652 | 679 | 0.96:1 |
FormMinimalPerf.default | 218 | 226 | 0.96:1 |
HeaderMinimalPerf.default | 201 | 209 | 0.96:1 |
ListWith60ListItems.default | 356 | 369 | 0.96:1 |
ReactionMinimalPerf.default | 203 | 214 | 0.95:1 |
LabelMinimalPerf.default | 206 | 218 | 0.94:1 |
00104fd
to
8df2d5b
Compare
🕵 fluentuiv8 No visual regressions between this PR and main |
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 9bfaadf:
|
Perf Analysis (
|
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
BaseButton | mount | 618 | 651 | 5000 | |
Breadcrumb | mount | 1733 | 1689 | 1000 | |
Checkbox | mount | 1693 | 1684 | 5000 | |
CheckboxBase | mount | 1474 | 1467 | 5000 | |
ChoiceGroup | mount | 2890 | 2871 | 5000 | |
ComboBox | mount | 655 | 642 | 1000 | |
CommandBar | mount | 6234 | 6274 | 1000 | |
ContextualMenu | mount | 13107 | 14183 | 1000 | |
DefaultButton | mount | 764 | 743 | 5000 | |
DetailsRow | mount | 2185 | 2155 | 5000 | |
DetailsRowFast | mount | 2145 | 2178 | 5000 | |
DetailsRowNoStyles | mount | 1986 | 1973 | 5000 | |
Dialog | mount | 2787 | 2657 | 1000 | |
DocumentCardTitle | mount | 234 | 232 | 1000 | |
Dropdown | mount | 1966 | 1945 | 5000 | |
FocusTrapZone | mount | 1122 | 1131 | 5000 | |
FocusZone | mount | 1041 | 1082 | 5000 | |
GroupedList | mount | 41859 | 41554 | 2 | |
GroupedList | virtual-rerender | 17889 | 19962 | 2 | |
GroupedList | virtual-rerender-with-unmount | 51199 | 50961 | 2 | |
GroupedListV2 | mount | 229 | 240 | 2 | |
GroupedListV2 | virtual-rerender | 216 | 215 | 2 | |
GroupedListV2 | virtual-rerender-with-unmount | 232 | 238 | 2 | |
IconButton | mount | 1097 | 1101 | 5000 | |
Label | mount | 327 | 336 | 5000 | |
Layer | mount | 2767 | 2683 | 5000 | |
Link | mount | 398 | 388 | 5000 | |
MenuButton | mount | 904 | 922 | 5000 | |
MessageBar | mount | 21461 | 21549 | 5000 | |
Nav | mount | 1921 | 1946 | 1000 | |
OverflowSet | mount | 780 | 780 | 5000 | |
Panel | mount | 1805 | 1733 | 1000 | |
Persona | mount | 737 | 739 | 1000 | |
Pivot | mount | 884 | 866 | 1000 | |
PrimaryButton | mount | 843 | 845 | 5000 | |
Rating | mount | 4589 | 4577 | 5000 | |
SearchBox | mount | 917 | 923 | 5000 | |
Shimmer | mount | 1911 | 1839 | 5000 | |
Slider | mount | 1329 | 1308 | 5000 | |
SpinButton | mount | 2901 | 2779 | 5000 | |
Spinner | mount | 391 | 384 | 5000 | |
SplitButton | mount | 1877 | 1857 | 5000 | |
Stack | mount | 413 | 411 | 5000 | |
StackWithIntrinsicChildren | mount | 862 | 847 | 5000 | |
StackWithTextChildren | mount | 2564 | 2579 | 5000 | |
SwatchColorPicker | mount | 6193 | 6170 | 5000 | |
TagPicker | mount | 1474 | 1495 | 5000 | |
Text | mount | 380 | 369 | 5000 | |
TextField | mount | 926 | 917 | 5000 | |
ThemeProvider | mount | 819 | 824 | 5000 | |
ThemeProvider | virtual-rerender | 592 | 577 | 5000 | |
ThemeProvider | virtual-rerender-with-unmount | 1284 | 1273 | 5000 | |
Toggle | mount | 616 | 616 | 5000 | |
buttonNative | mount | 191 | 195 | 5000 |
🕵 FluentUIV0 No visual regressions between this PR and main |
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 300073896d3b7ab9a0556e5b1b886d46c99d2515 (build) |
🕵 fluentuiv9 No visual regressions between this PR and main |
cb00157
to
ce73e62
Compare
4884af4
to
c1324de
Compare
@@ -0,0 +1,104 @@ | |||
import * as ts from 'typescript'; |
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 copy-pasted from the deleted part in getCallbackArguments. I don't understand it enough to make any change :/
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.
It would be good idea to actually dissect these changes into multiple PRs for sake of granularity.
Regarding the conformance implementation:
can we remove consistent-callback-args
from conformance defaults instead ?
- removing all disabled definition for that check + adding that check only where it is actually used ( which based on this PR is focus zone/react v8 only ? )
- no renaming churn needed
- marking it as
@deprecated
@@ -0,0 +1,7 @@ | |||
{ | |||
"type": "minor", |
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.
As you renaming consistent-callback-args
, it should be a major bump (as it's a breaking change).
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.
react-conformance is on version 0.x.x. I think minor here is for breaking change.
@Hotell I'm in favor of removing consistent-callback-args. But it is also in use by most of the v9 components, and I would like to keep it this way. |
f6c7185
to
4a28221
Compare
'consistent-callback-args-legacy': { | ||
ignoreProps: ['onActiveElementChanged', 'onBeforeFocus', 'onFocusNotification', 'onFocus'], | ||
}, | ||
'consistent-callback-type': { |
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.
why is this being added to v8 package ?
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.
Because we want to disable the new consistent-callback-type
for v8.
If you meant consistent-callback-args-legacy
, the git diff is not very smart there, it was already in v8.
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.
disabledTests: ['make-styles-overrides-win', 'consistent-callback-args-legacy'], | ||
testOptions: { | ||
'consistent-callback-type': { | ||
ignoreProps: ['onToggle'], |
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.
why are some handlers being ignored ? what's the plan here , will it be fixed in a follow up PR ?
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.
The plan is to enable the new type EventHandler
only for new v9 components, or newly added event callback props. Because we can't break the type of the existing props.
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.
The existing callbacks will be verified by the legacy test consistent-callback-args-legacy
.
The newly added callbacks will be verified by consistent-callback-type
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.
so existing v9 packages (this one in particular), doesnt contain any old callback types ?
Gotcha, my assumption was based on this PR changes, havent checked all v9 packages.
It's bit unfortunated that we keep extending react-conformance even we know we wanna get rid of it because its architecture limitations and performance bottlenecks. I'm wondering if you could add this new functionality as an eslint rule which is what conformance will be migrated to in future ? Lets't not expand public API surface of this package because the reasons mentioned. I propose following (for the old test case rule):
![]() |
disabledTests: ['make-styles-overrides-win', 'consistent-callback-args-legacy'], | ||
testOptions: { | ||
'consistent-callback-type': { | ||
ignoreProps: ['onToggle'], |
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.
The way these conformance tests are set up seems like a configuration headache. It's confusing when the -legacy
version should be enabled. It's also not obvious why some props are being ignored, and when/if new props should be ignored as well.
IMO we should keep a single consistent-callback-args
check: don't split it into two tests that have to be enabled/disabled in different scenarios. The test could be configured to check legacy callbacks against the legacy pattern (rather than simply ignored).
E.g.
disabledTests: ['make-styles-overrides-win', 'consistent-callback-args-legacy'], | |
testOptions: { | |
'consistent-callback-type': { | |
ignoreProps: ['onToggle'], | |
disabledTests: ['make-styles-overrides-win'], | |
testOptions: { | |
'consistent-callback-args': { | |
legacyCallbacks: ['onToggle'], |
The config option could be named legacyCallbacks_doNotAddNewPropsHere
or something, to make it even clearer.
I guess we can close this one ? |
Background:
We agreed on new event type for v9 callbacks.
Below types will be exported from react-utilities as helpers:
And they will be used for typing callbacks:
detailed proposal: https://github.com/microsoft/fluentui/blob/master/rfcs/react-components/convergence/event-handlers-event-type.md
This PR does: