feat(react-conformance): add test for callback arguments by layershifter · Pull Request #19369 · microsoft/fluentui

layershifter · 2021-08-11T14:29:33Z

Pull request checklist

Addresses an existing issue: Related to Implementation of RFC: standardize event handlers arguments #19335
Include a change request file using $ yarn change

Description of changes

This PR adds first test for callback arguments to ensure that #18974 is implemented properly.

Adds test for number of arguments in on* callbacks (already found issues in Menu* components 😉)

interface ComponentProps {
  // ✅ has two params
  onChange: (event, data) => void;

  // ❌ breaks as has three params
  onChange: (event, data, value) => void;
}

Disables new test for Menu*, FocusZone, N* & v9 tests
Adds custom AST processing 😥

Custom AST processing

In conformance tests we use information about components generated by react-typescript-docgen, unfortunately it stringifies information about types:

{
  "name": "onToggle",
  "required": false,
  "type": { "name": "AccordionToggleEventHandler | undefined" }
}

{
  "name": "onVisibleChange",
  "required": false,
  "type": {
    "name": "((event: FocusEvent<HTMLElement> | PointerEvent<HTMLElement> | undefined,
 data: OnVisibleChangeData) => void) | undefined"
  }
}

👆 output is simplified

type.name is just a string
From that string we can't get any information (Accordion's case, inTooltip`'s case it's better, but we will need to parse strings)
You cannot change this behavior, react-docgen-typescript calls checker.typeToString() that stringifies type:
https://github.com/styleguidist/react-docgen-typescript/blob/40edb172243ed3a431e1346bda1bd74c929af6d4/src/parser.ts#L606

Taking this into account I created additional type parsing via TS AST, see getCallbackArguments.ts. This function takes details about an interface and props and returns arguments in readable format. For example:

import * as React from "react";

export interface AccordionProps {
  onToggle: (e: React.MouseEvent) => void;
}

getCallbackArguments(
  program,
  "Accordion.types.ts",
  "AccordionProps",
  "onToggle"
); // ➡ { e: 'React.MouseEvent' }

Data provided by this function are used by conformance tests: now we can ensure that number of arguments, their order & types are correct.

For Northstar, we have similar post-processing, microsoft/fluent-ui-react#1634.

Minor changes

createTsProgram() was moved from getComponentDoc() so it can be reused. We don't want to create new instances for performance reasons

Follow ups

Improve AST parse to resolve nested types, for example: type AccordionToggleEventHandler = (event: AccordionToggleEvent, data: AccordionToggleData) => void
Add new test that ensures that types of arguments are correct

codesandbox-ci · 2021-08-11T14:39:03Z

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 f5ed1ea:

Sandbox	Source
Fluent UI React Starter	Configuration

size-auditor · 2021-08-11T14:41:22Z

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: 7dfbf1225a317a7a930a177b5f4674add1322ad7 (build)

fabricteam · 2021-08-11T14:44:16Z

📊 Bundle size report

Unchanged fixtures

Package & Exports	Size (minified/GZIP)
`react-accordion` Accordion (including children components)	`55.21 kB` `17.401 kB`
`react-avatar` Avatar	`56.558 kB` `15.66 kB`
`react-badge` Badge	`24.343 kB` `7.165 kB`
`react-badge` CounterBadge	`27.156 kB` `7.851 kB`
`react-badge` PresenseBadge	`237 B` `177 B`
`react-button` Button	`22.932 kB` `6.984 kB`
`react-button` CompoundButton	`28.215 kB` `7.834 kB`
`react-button` MenuButton	`24.733 kB` `7.546 kB`
`react-button` ToggleButton	`32.527 kB` `7.601 kB`
`react-components` react-components: Accordion, Button, FluentProvider, Image, Menu, Popover	`164.392 kB` `46.919 kB`
`react-components` react-components: FluentProvider & webLightTheme	`36.258 kB` `11.596 kB`
`react-divider` Divider	`15.889 kB` `5.747 kB`
`react-image` Image	`10.642 kB` `4.264 kB`
`react-input` Input	`31.636 kB` `11.312 kB`
`react-label` Label	`9.397 kB` `3.839 kB`
`react-link` Link	`12.609 kB` `4.948 kB`
`react-menu` Menu (including children components)	`103.923 kB` `31.598 kB`
`react-menu` Menu (including selectable components)	`106.683 kB` `32.149 kB`
`react-popover` Popover	`100.644 kB` `30.142 kB`
`react-portal` Portal	`7.78 kB` `2.672 kB`
`react-provider` FluentProvider	`16.256 kB` `5.969 kB`
`react-slider` Slider	`30.269 kB` `9.561 kB`
`react-text` Text - Default	`11.798 kB` `4.452 kB`
`react-text` Text - Wrappers	`15.414 kB` `4.734 kB`
`react-tooltip` Tooltip	`46.054 kB` `15.658 kB`

_{🤖 This report was generated against 7dfbf1225a317a7a930a177b5f4674add1322ad7}

fabricteam · 2021-08-11T16:29:55Z

Perf Analysis (`@fluentui/react`)

No significant results to display.

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations
Avatar	mount	963	980	5000
BaseButton	mount	993	965	5000
Breadcrumb	mount	2650	2650	1000
ButtonNext	mount	469	481	5000
Checkbox	mount	1673	1781	5000
CheckboxBase	mount	1393	1409	5000
ChoiceGroup	mount	5060	5147	5000
ComboBox	mount	1021	1005	1000
CommandBar	mount	10476	10809	1000
ContextualMenu	mount	6596	6248	1000
DefaultButton	mount	1196	1203	5000
DetailsRow	mount	3900	3944	5000
DetailsRowFast	mount	3964	3986	5000
DetailsRowNoStyles	mount	3824	3802	5000
Dialog	mount	2232	2226	1000
DocumentCardTitle	mount	141	150	1000
Dropdown	mount	3486	3410	5000
FluentProviderNext	mount	7094	7513	5000
FluentProviderWithTheme	mount	388	339	10
FluentProviderWithTheme	virtual-rerender	101	100	10
FluentProviderWithTheme	virtual-rerender-with-unmount	478	480	10
FocusTrapZone	mount	1899	1878	5000
FocusZone	mount	1860	1955	5000
IconButton	mount	1878	1858	5000
Label	mount	356	361	5000
Layer	mount	1867	1906	5000
Link	mount	491	482	5000
MakeStyles	mount	1899	1915	50000
MenuButton	mount	1609	1616	5000
MessageBar	mount	2209	2214	5000
Nav	mount	3762	3652	1000
OverflowSet	mount	1118	1136	5000
Panel	mount	2170	2127	1000
Persona	mount	898	867	1000
Pivot	mount	1467	1467	1000
PrimaryButton	mount	1326	1379	5000
Rating	mount	8453	8644	5000
SearchBox	mount	1466	1457	5000
Shimmer	mount	2766	2809	5000
Slider	mount	2129	2116	5000
SpinButton	mount	5343	5355	5000
Spinner	mount	429	425	5000
SplitButton	mount	3331	3250	5000
Stack	mount	552	549	5000
StackWithIntrinsicChildren	mount	1703	1749	5000
StackWithTextChildren	mount	5002	5134	5000
SwatchColorPicker	mount	10884	10920	5000
Tabs	mount	1485	1437	1000
TagPicker	mount	2747	2760	5000
TeachingBubble	mount	12169	12314	5000
Text	mount	451	458	5000
TextField	mount	1501	1559	5000
ThemeProvider	mount	1240	1249	5000
ThemeProvider	virtual-rerender	606	626	5000
ThemeProvider	virtual-rerender-with-unmount	2065	2039	5000
Toggle	mount	840	861	5000
buttonNative	mount	125	118	5000

Perf Analysis (`@fluentui/react-northstar`)

Perf tests with no regressions

Scenario	Current PR Ticks	Baseline Ticks	Ratio
RefMinimalPerf.default	246	226	1.09:1
ImageMinimalPerf.default	456	424	1.08:1
TreeWith60ListItems.default	195	180	1.08:1
AnimationMinimalPerf.default	483	451	1.07:1
ListMinimalPerf.default	582	543	1.07:1
MenuMinimalPerf.default	971	905	1.07:1
PortalMinimalPerf.default	183	171	1.07:1
ButtonMinimalPerf.default	194	183	1.06:1
ChatWithPopoverPerf.default	408	385	1.06:1
HeaderSlotsPerf.default	909	855	1.06:1
AlertMinimalPerf.default	300	287	1.05:1
AttachmentMinimalPerf.default	180	171	1.05:1
ChatMinimalPerf.default	750	711	1.05:1
DropdownManyItemsPerf.default	776	738	1.05:1
FlexMinimalPerf.default	330	315	1.05:1
PopupMinimalPerf.default	650	619	1.05:1
ButtonSlotsPerf.default	597	573	1.04:1
HeaderMinimalPerf.default	406	391	1.04:1
ReactionMinimalPerf.default	435	417	1.04:1
SkeletonMinimalPerf.default	397	382	1.04:1
VideoMinimalPerf.default	729	702	1.04:1
SliderMinimalPerf.default	1699	1654	1.03:1
CardMinimalPerf.default	629	618	1.02:1
ChatDuplicateMessagesPerf.default	326	321	1.02:1
FormMinimalPerf.default	478	470	1.02:1
ListNestedPerf.default	621	611	1.02:1
RadioGroupMinimalPerf.default	487	478	1.02:1
SegmentMinimalPerf.default	381	373	1.02:1
StatusMinimalPerf.default	781	766	1.02:1
TableMinimalPerf.default	476	468	1.02:1
TextAreaMinimalPerf.default	559	549	1.02:1
AttachmentSlotsPerf.default	1163	1157	1.01:1
ItemLayoutMinimalPerf.default	1333	1325	1.01:1
LabelMinimalPerf.default	436	431	1.01:1
ListCommonPerf.default	700	690	1.01:1
MenuButtonMinimalPerf.default	1781	1763	1.01:1
ProviderMergeThemesPerf.default	1722	1709	1.01:1
ProviderMinimalPerf.default	1092	1078	1.01:1
TextMinimalPerf.default	383	380	1.01:1
BoxMinimalPerf.default	388	389	1:1
ButtonOverridesMissPerf.default	1874	1866	1:1
CheckboxMinimalPerf.default	2901	2904	1:1
DialogMinimalPerf.default	840	837	1:1
DividerMinimalPerf.default	401	401	1:1
GridMinimalPerf.default	406	405	1:1
ListWith60ListItems.default	723	723	1:1
TableManyItemsPerf.default	2130	2140	1:1
CustomToolbarPrototype.default	4032	4039	1:1
ToolbarMinimalPerf.default	1017	1015	1:1
TooltipMinimalPerf.default	1091	1093	1:1
TreeMinimalPerf.default	854	857	1:1
DropdownMinimalPerf.default	3340	3376	0.99:1
InputMinimalPerf.default	1367	1379	0.99:1
LoaderMinimalPerf.default	740	747	0.99:1
RosterPerf.default	1294	1303	0.99:1
CarouselMinimalPerf.default	503	513	0.98:1
EmbedMinimalPerf.default	4584	4655	0.98:1
SplitButtonMinimalPerf.default	4537	4635	0.98:1
DatepickerMinimalPerf.default	5767	5973	0.97:1
LayoutMinimalPerf.default	391	406	0.96:1
AccordionMinimalPerf.default	170	180	0.94:1
AvatarMinimalPerf.default	212	226	0.94:1
IconMinimalPerf.default	663	710	0.93:1

DustyTheBot · 2021-08-11T17:16:47Z

	Warnings
⚠️	There are no updates provided to CHANGELOG. Ensure there are no publicly visible changes introduced by this PR.

Generated by 🚫 dangerJS against f5ed1ea

packages/react-conformance/src/defaultErrorMessages.tsx

packages/react-conformance/src/defaultTests.tsx

ling1726 · 2021-08-31T10:15:25Z

packages/react-conformance/src/defaultTests.tsx

+          const handlerArguments = getCallbackArguments(tsProgram, rootFileName, typeName, propName);
+
+          if (Object.keys(handlerArguments).length !== 2) {
+            return true;


It should be possible to assert that both arguments are objects, since you parse the types already ? or at least they aren't primitites like string, number

This will be done in a follow up PR as I need to improve resolving logic first (see commented test)

packages/react-conformance/src/utils/getCallbackArguments.test.ts

ling1726 · 2021-08-31T10:28:50Z

packages/react-conformance/src/utils/getCallbackArguments.test.ts

+      expect(type).toMatchObject({ a: 'String', b: 'Number', c: 'Boolean', d: undefined });
+    });
+
+    it('handles arrays', async () => {


nit: it kind of seems like a lot of copy pasted test structure, since you always provide the same structure of input and assertion, it would be nice to use parametrized test like it.each

I agree, but due size of fixtures I would like to leave them as it is. It also easier to use it.only to debug failing tests

packages/react-conformance/src/utils/getCallbackArguments.ts

khmakoto

This looks awesome! I just want to echo some of @ling1726's comments and one other minor comment but otherwise looks good to me and would be more than happy to approve once those are addressed.

packages/react-conformance/src/utils/getCallbackArguments.test.ts

packages/react-conformance/src/defaultErrorMessages.tsx

ecraig12345

This is great, and sorry for the delayed review! I have some comments but nothing blocking.

packages/react-conformance/src/defaultTests.tsx

ecraig12345 · 2021-09-01T23:08:13Z

packages/react-conformance/src/defaultTests.tsx

+                `Definition for "${propName}" does not have ".declarations".`,
+                'Please report a bug if this happens',


I'm not sure this message will be meaningful to someone who hasn't been immersed in typescript AST analysis, or that it will be clear who to notify or what the bug should contain.

I tried to improve it to be more meaningful. Let me know what do you think

ecraig12345 · 2021-09-01T23:11:29Z

packages/react-conformance/src/utils/createTsProgram.ts

+let program: ts.Program;
+
+/**
+ * Creates a cached TS Program.


This isn't an issue new in this PR, but I realized a couple months ago that due to the way Jest runs tests (separate per file), the program is actually not cached across components. I'm surprised and a bit confused that I didn't notice this during the initial implementation. 😬

It's fine to leave this as-is for now, but ultimately I think to increase test performance, we may need to split all typing-based tests into a separate test suite (maybe in its own package) that invokes typing-related tests on all components at once to get rid of the duplicate analysis. I started work on that back when I noticed the issue but unfortunately haven't gotten to finish. (I can share what I have so far--actually one of the holdups was figuring out the "right" design, so feedback on that would be great.)

Made an issue #19697 also with a link to my branch with the unfinished (and outdated) changes

packages/react-conformance/src/utils/getCallbackArguments.ts

…eat/conformance-callbacks � Conflicts: � packages/react-conformance/src/isConformant.ts � packages/react-menu/src/components/MenuList/MenuList.test.tsx

…eat/conformance-callbacks

layershifter · 2021-09-06T11:26:38Z

/azp run

azure-pipelines · 2021-09-06T11:27:12Z

Azure Pipelines successfully started running 3 pipeline(s).

change/@fluentui-react-conformance-1461c075-e920-4b98-ad27-2adbf47103e0.json

…bf47103e0.json

layershifter added 2 commits August 11, 2021 16:26

feat(react-conformance): add test for callback arguments

e7ce330

Change files

916c025

layershifter added the Fluent UI react-components (v9) label Aug 11, 2021

layershifter added this to the August Project Cycle Q3 2021 milestone Aug 11, 2021

layershifter added 2 commits August 11, 2021 16:42

lint fixes

1df49ca

remove cast

acf513c

layershifter added 7 commits August 11, 2021 17:01

handle undefined

dacd71d

disable tests in FocusZone

93c9fa6

Change files

071ead4

handle arrays

d8f13d2

disable test for MenuList

2681dee

Change files

c493df3

disable test for Menu

cce816d

layershifter added 3 commits August 11, 2021 18:53

disable tests

3ed0d73

Change files

60f1034

disable test in N*

e8edc83

layershifter mentioned this pull request Aug 12, 2021

Implementation of RFC: standardize event handlers arguments #19335

Closed

4 tasks

layershifter added 2 commits August 12, 2021 11:00

add error messages

d09ac33

update changelog entries

9891f09

layershifter marked this pull request as ready for review August 12, 2021 09:19

layershifter requested review from jurokapsiar, khmakoto, ling1726, miroslavstastny and pompomon as code owners August 12, 2021 09:19

layershifter requested a review from a team as a code owner August 12, 2021 09:19

layershifter requested a review from ecraig12345 August 12, 2021 09:20