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

Add conformance test for event callback typing: consistent-callback-type #30200

Closed
wants to merge 9 commits into from
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "minor",
Copy link
Member

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).

Copy link
Contributor Author

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.

"comment": "feat: add 'consistent-callback-type' test, rename 'consistent-callback-args' to 'consistent-callback-args-legacy'",
"packageName": "@fluentui/react-conformance",
"email": "[email protected]",
"dependentChangeType": "patch"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "minor",
"comment": "feat: add type helper for callback props",
"packageName": "@fluentui/react-utilities",
"email": "[email protected]",
"dependentChangeType": "patch"
}
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,8 @@ export function isConformant(
// v0 doesn't use the patterns these tests look at
disabledTests: [
'has-top-level-file',
'consistent-callback-args',
'consistent-callback-args-legacy',
'consistent-callback-type',
'component-has-static-classname',
'component-has-static-classnames-object',
'component-has-static-classname-exported',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,12 @@ describe('Accordion', () => {
Component: Accordion,
displayName: 'Accordion',
// Accordion does not have own styles
disabledTests: ['make-styles-overrides-win', 'consistent-callback-args'],
disabledTests: ['make-styles-overrides-win', 'consistent-callback-args-legacy'],
testOptions: {
'consistent-callback-type': {
ignoreProps: ['onToggle'],
Copy link
Contributor

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 ?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

Copy link
Contributor

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 ?

Comment on lines +11 to +14
Copy link
Contributor

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.

Suggested change
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.

},
},
});

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ describe('AvatarGroupPopover', () => {
getPortalElement: getPopoverSurfaceElement,
},
],
'consistent-callback-type': {
ignoreProps: ['onOpenChange'],
},
},
requiredProps: {
children: (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,13 @@ describe('Calendar', () => {
displayName: 'Calendar',
disabledTests: [
// compat components that are closer to their v8 counterparts do not adhere to this test
'consistent-callback-args',
'consistent-callback-args-legacy',
// Calendar is not exported at the top level since it's an internal component
'exported-top-level',
// Calendar classnames are not exported since they are internal and are used differently compared to how v9
// uses classnames
'component-has-static-classnames-object',
'consistent-callback-type',
],
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,10 @@ describe('CalendarDayGrid', () => {
'component-handles-ref',
'component-has-root-ref',
// This test doesn't apply for compat components that are closer to their v8 counterpart
'consistent-callback-args',
'consistent-callback-args-legacy',
// Some classnames are applied conditionally
'component-has-static-classnames-object',
'consistent-callback-type',
],
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ describe('Card', () => {
},
},
],
'consistent-callback-type': {
ignoreProps: ['onSelectionChange'],
},
},
disabledTests: ['component-has-static-classname-exported'],
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ describe('Checkbox', () => {
props: { label: 'Test Label' },
},
],
'consistent-callback-type': {
ignoreProps: ['onChange'],
},
},
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ describe('Combobox', () => {
},
},
],
'consistent-callback-type': {
ignoreProps: ['onOptionSelect', 'onOpenChange'],
},
},
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ describe('Dropdown', () => {
},
},
],
'consistent-callback-type': {
ignoreProps: ['onOptionSelect', 'onOpenChange'],
},
},
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@ describe('Listbox', () => {
isConformant({
Component: Listbox,
displayName: 'Listbox',
testOptions: {
'consistent-callback-type': {
ignoreProps: ['onOptionSelect'],
},
},
});

it('renders a default state', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ describe('DatePicker', () => {
displayName: 'DatePicker',
// component-has-root-ref is disabled because the root is an Input component, the conformance test thinks the
// wrapper is the root, not the input itself. This is a bug in the conformance test.
disabledTests: ['consistent-callback-args', 'component-has-root-ref'],
disabledTests: ['consistent-callback-args-legacy', 'consistent-callback-type', 'component-has-root-ref'],
testOptions: {
'has-static-classnames': [
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,15 @@ describe('Dialog', () => {
'component-has-static-classnames-object',
// TODO:
// onOpenChange: A second (data) argument cannot be a union
'consistent-callback-args',
'consistent-callback-args-legacy',
// Dialog does not have own styles
'make-styles-overrides-win',
],
testOptions: {
'consistent-callback-type': {
ignoreProps: ['onOpenChange'],
},
},
});

it('renders a default state', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ describe('Drawer', () => {
* Why these tests are disabled:
* component-handles-ref|component-has-root-ref: Drawer uses the Dialog under the hood and Dialog do not handle ref, as it is a renderless component
* component-handles-classname|component-has-static-classnames-object|make-styles-overrides-win: Drawer uses the DialogSurface component to render the classname, so the main component do not handle classname.
* consistent-callback-args: Disabled that as the Drawer callback function uses the same signature as the Dialog, and Dialog has those tests disabled.
* consistent-callback-args-legacy: Disabled that as the Drawer callback function uses the same signature as the Dialog, and Dialog has those tests disabled.
*/
isConformant({
Component: Drawer,
Expand All @@ -32,11 +32,16 @@ describe('Drawer', () => {
'component-has-root-ref',
'component-handles-classname',
'component-has-static-classnames-object',
'consistent-callback-args',
'consistent-callback-args-legacy',
'make-styles-overrides-win',
],
requiredProps: props,
getTargetElement: result => result.getByTestId(testid),
testOptions: {
'consistent-callback-type': {
ignoreProps: ['onOpenChange'],
},
},
});

it('renders a default state', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ describe('OverlayDrawer', () => {
* Why these tests are disabled:
* component-handles-ref|component-has-root-ref: OverlayDrawer uses the Dialog under the hood and Dialog do not handle ref, as it is a renderless component
* component-handles-classname|component-has-static-classnames-object|make-styles-overrides-win: OverlayDrawer uses the DialogSurface component to render the className, so the main component do not handle className.
* consistent-callback-args: Disabled that as the OverlayDrawer callback function uses the same signature as the Dialog, and Dialog has those tests disabled.
* consistent-callback-args-legacy: Disabled that as the OverlayDrawer callback function uses the same signature as the Dialog, and Dialog has those tests disabled.
*/
isConformant({
Component: OverlayDrawer,
Expand All @@ -30,10 +30,15 @@ describe('OverlayDrawer', () => {
'component-has-root-ref',
'component-handles-classname',
'component-has-static-classnames-object',
'consistent-callback-args',
'consistent-callback-args-legacy',
],
requiredProps: props,
getTargetElement: result => result.getByTestId(testid),
testOptions: {
'consistent-callback-type': {
ignoreProps: ['onOpenChange'],
},
},
});

it('renders a default state', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ describe('Input', () => {
},
},
],
'consistent-callback-type': {
ignoreProps: ['onChange'],
},
},
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ describe('Menu', () => {
'make-styles-overrides-win',
// TODO:
// onOpenChange: A second (data) argument cannot be a union
'consistent-callback-args',
'consistent-callback-args-legacy',
],
Component: Menu,
displayName: 'Menu',
Expand All @@ -39,6 +39,11 @@ describe('Menu', () => {
</MenuPopover>,
],
},
testOptions: {
'consistent-callback-type': {
ignoreProps: ['onCheckedValueChange', 'onOpenChange'],
},
},
});

beforeEach(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ describe('MenuList', () => {
// MenuTrigger does not have own styles
'make-styles-overrides-win',
],
testOptions: {
'consistent-callback-type': {
ignoreProps: ['onCheckedValueChange'],
},
},
});

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,13 @@ describe('List', () => {
// Adding them there now might not be safe.
disabledTests: ['component-has-static-classnames-object', 'has-docblock', 'has-top-level-file'],
testOptions: {
'consistent-callback-args': {
'consistent-callback-args-legacy': {
// onSelectionChange has an eventArgument which is React.SyntheticEvent. This throws an error during testing
ignoreProps: ['onSelectionChange'],
},
'consistent-callback-type': {
ignoreProps: ['onSelectionChange'],
},
},
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,6 @@ describe('Nav', () => {
Component: Nav,
displayName: 'Nav',
// todo - # 30012, remove when conformance is updated
disabledTests: ['consistent-callback-args'],
disabledTests: ['consistent-callback-args-legacy'],
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ describe('Popover', () => {
// Popover does not have own styles
'make-styles-overrides-win',
],
testOptions: {
'consistent-callback-type': {
ignoreProps: ['onOpenChange'],
},
},
});

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ describe('Radio', () => {
},
},
],
'consistent-callback-type': {
ignoreProps: ['onChange'],
},
},
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@ describe('RadioGroup', () => {
isConformant({
Component: RadioGroup,
displayName: 'RadioGroup',
testOptions: {
'consistent-callback-type': {
ignoreProps: ['onChange'],
},
},
});

it('renders a default state', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ describe('SearchBox', () => {
},
},
],
'consistent-callback-type': {
ignoreProps: ['onChange'],
},
},
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,11 @@ describe('Select', () => {
Component: Select,
displayName: 'Select',
primarySlot: 'select',
testOptions: {
'consistent-callback-type': {
ignoreProps: ['onChange'],
},
},
});

// Note for Select tests: avoid using getByRole;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@ describe('Slider', () => {
Component: Slider,
displayName: 'Slider',
primarySlot: 'input',
testOptions: {
'consistent-callback-type': {
ignoreProps: ['onChange'],
},
},
});

afterEach(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ describe('SpinButton', () => {
Component: SpinButton,
displayName: 'SpinButton',
primarySlot: 'input',
testOptions: {
'consistent-callback-type': {
ignoreProps: ['onChange'],
},
},
});

it('renders a default uncontrolled state', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ describe('Switch', () => {
},
},
],
'consistent-callback-type': {
ignoreProps: ['onChange'],
},
},
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ describe('DataGrid', () => {
'make-styles-overrides-win': {
callCount: 2,
},
'consistent-callback-type': {
ignoreProps: ['onSortChange', 'onSelectionChange', 'onColumnResize'],
},
},
// TODO: https://github.com/microsoft/fluentui/issues/19618
// eslint-disable-next-line @typescript-eslint/no-explicit-any
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@ describe('TabList', () => {
isConformant({
Component: TabList,
displayName: 'TabList',
testOptions: {
'consistent-callback-type': {
ignoreProps: ['onTabSelect'],
},
},
});

it('renders with tabs', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,12 @@ describe('TagGroup', () => {
Component: TagGroup,
displayName: 'TagGroup',
testOptions: {
'consistent-callback-args': {
'consistent-callback-args-legacy': {
ignoreProps: ['onDismiss'], // onDismiss uses generics, this test does not support that
},
'consistent-callback-type': {
ignoreProps: ['onDismiss'],
},
},
});

Expand Down
Loading
Loading