Add eslint-plugin-jsdoc to check jsdoc comments by behowell · Pull Request #19271 · microsoft/fluentui

behowell · 2021-08-05T03:07:14Z

Pull request checklist

Include a change request file using $ yarn change

Description of changes

This change adds the jsdoc plugin for eslint. For now, only the check-tag-names rule is enabled, and temporary tagNamePreference values are added to avoid causing errors yet. In a future PR, I plan to update the config to require @default instead of @defaultvalue, and @returns instead of @return.

See #18566 (comment) for reference about plans to change @defaultvalue to @default

…dd-eslint-plugin-jsdoc

behowell · 2021-08-05T03:07:59Z

+      tagNamePreference: {
+        defaultvalue: 'defaultvalue',
+        defaultValue: 'defaultValue',
+        return: 'return',
+      },


These are temporary and will be removed in a future PR. I wanted to scope this PR to adding the new plugin, and a future PR to fix places that use @defaultvalue

can we move to this to generic config ? every platform should use same conventions (node/web)

Right now the node config actually extends from this one and just turns off no-console (which is maybe not great, but a separate problem)

behowell · 2021-08-05T03:09:32Z

+  "comment": "Add eslint-plugin-jsdoc to check for jsdoc errors",
+  "packageName": "@fluentui/eslint-plugin",
+  "email": "[email protected]",
+  "dependentChangeType": "none"


Note I changed this to none, per https://github.com/microsoft/fluentui/pull/19169/files#r681520686

great catch @behowell !

@ecraig12345 do we need to publish this package at all? who consumes this outside our org ?
if we make it private then we can mitigate such a chore that happened last time with releases.

WDYT?

IIRC I originally made it public because some of the custom rules (outlined in the readme) might be useful to others. But I have no way to know if anybody else has actually found and used them.

codesandbox-ci · 2021-08-05T03:14:14Z

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

Sandbox	Source
Fluent UI React Starter	Configuration

fabricteam · 2021-08-05T03:34:55Z

Perf Analysis (`@fluentui/react`)

No significant results to display.

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations
Avatar	mount	965	1011	5000
BaseButton	mount	969	962	5000
Breadcrumb	mount	2639	2599	1000
ButtonNext	mount	468	447	5000
Checkbox	mount	1714	1648	5000
CheckboxBase	mount	1430	1416	5000
ChoiceGroup	mount	5153	5082	5000
ComboBox	mount	1006	1012	1000
CommandBar	mount	10543	10863	1000
ContextualMenu	mount	6483	6287	1000
DefaultButton	mount	1233	1243	5000
DetailsRow	mount	3909	3996	5000
DetailsRowFast	mount	3973	3936	5000
DetailsRowNoStyles	mount	3669	3806	5000
Dialog	mount	2399	2249	1000
DocumentCardTitle	mount	165	161	1000
Dropdown	mount	3619	3612	5000
FluentProviderNext	mount	7204	7378	5000
FocusTrapZone	mount	1883	1858	5000
FocusZone	mount	1871	1850	5000
IconButton	mount	2186	1944	5000
Label	mount	361	355	5000
Layer	mount	1867	1848	5000
Link	mount	492	486	5000
MakeStyles	mount	1842	1895	50000
MenuButton	mount	1556	1587	5000
MessageBar	mount	2060	2122	5000
Nav	mount	3515	3423	1000
OverflowSet	mount	1184	1134	5000
Panel	mount	2120	2141	1000
Persona	mount	865	888	1000
Pivot	mount	1575	1465	1000
PrimaryButton	mount	1353	1380	5000
Rating	mount	8415	8459	5000
SearchBox	mount	1444	1506	5000
Shimmer	mount	2779	2714	5000
Slider	mount	2033	2071	5000
SpinButton	mount	5297	5262	5000
Spinner	mount	432	438	5000
SplitButton	mount	3359	3324	5000
Stack	mount	529	533	5000
StackWithIntrinsicChildren	mount	1693	1656	5000
StackWithTextChildren	mount	5015	5082	5000
SwatchColorPicker	mount	11034	11047	5000
Tabs	mount	1481	1499	1000
TagPicker	mount	2760	2865	5000
TeachingBubble	mount	12468	12665	5000
Text	mount	442	454	5000
TextField	mount	1497	1502	5000
ThemeProvider	mount	1291	1313	5000
ThemeProvider	virtual-rerender	658	657	5000
Toggle	mount	882	909	5000
buttonNative	mount	114	120	5000

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

Perf tests with no regressions

Scenario	Current PR Ticks	Baseline Ticks	Ratio
ButtonMinimalPerf.default	198	176	1.13:1
CarouselMinimalPerf.default	530	487	1.09:1
DividerMinimalPerf.default	404	369	1.09:1
PopupMinimalPerf.default	648	598	1.08:1
AttachmentMinimalPerf.default	181	169	1.07:1
ChatMinimalPerf.default	739	696	1.06:1
GridMinimalPerf.default	373	354	1.05:1
TooltipMinimalPerf.default	1149	1097	1.05:1
AnimationMinimalPerf.default	443	428	1.04:1
LayoutMinimalPerf.default	429	412	1.04:1
PortalMinimalPerf.default	173	167	1.04:1
ReactionMinimalPerf.default	418	403	1.04:1
SkeletonMinimalPerf.default	378	364	1.04:1
StatusMinimalPerf.default	793	764	1.04:1
TextMinimalPerf.default	378	364	1.04:1
AttachmentSlotsPerf.default	1187	1149	1.03:1
AvatarMinimalPerf.default	207	200	1.03:1
BoxMinimalPerf.default	371	359	1.03:1
HeaderSlotsPerf.default	844	823	1.03:1
ListMinimalPerf.default	567	550	1.03:1
MenuMinimalPerf.default	912	889	1.03:1
ProviderMinimalPerf.default	1087	1057	1.03:1
TableManyItemsPerf.default	2151	2093	1.03:1
ToolbarMinimalPerf.default	1099	1067	1.03:1
ButtonOverridesMissPerf.default	1835	1795	1.02:1
DatepickerMinimalPerf.default	5876	5747	1.02:1
DropdownManyItemsPerf.default	738	723	1.02:1
EmbedMinimalPerf.default	4438	4369	1.02:1
FormMinimalPerf.default	462	455	1.02:1
HeaderMinimalPerf.default	393	386	1.02:1
SliderMinimalPerf.default	1656	1628	1.02:1
TableMinimalPerf.default	438	428	1.02:1
TextAreaMinimalPerf.default	551	541	1.02:1
ImageMinimalPerf.default	421	418	1.01:1
ItemLayoutMinimalPerf.default	1364	1350	1.01:1
ListCommonPerf.default	732	723	1.01:1
ListNestedPerf.default	611	606	1.01:1
ListWith60ListItems.default	698	690	1.01:1
LoaderMinimalPerf.default	725	715	1.01:1
RadioGroupMinimalPerf.default	476	473	1.01:1
SegmentMinimalPerf.default	376	372	1.01:1
CardMinimalPerf.default	604	607	1:1
CheckboxMinimalPerf.default	2823	2834	1:1
RefMinimalPerf.default	237	236	1:1
TreeMinimalPerf.default	842	844	1:1
VideoMinimalPerf.default	682	684	1:1
ButtonSlotsPerf.default	589	596	0.99:1
ChatDuplicateMessagesPerf.default	298	301	0.99:1
DialogMinimalPerf.default	820	832	0.99:1
LabelMinimalPerf.default	466	469	0.99:1
ProviderMergeThemesPerf.default	1675	1695	0.99:1
SplitButtonMinimalPerf.default	4150	4185	0.99:1
IconMinimalPerf.default	671	678	0.99:1
CustomToolbarPrototype.default	4052	4087	0.99:1
AccordionMinimalPerf.default	170	173	0.98:1
AlertMinimalPerf.default	297	304	0.98:1
ChatWithPopoverPerf.default	387	395	0.98:1
DropdownMinimalPerf.default	3186	3235	0.98:1
FlexMinimalPerf.default	309	314	0.98:1
RosterPerf.default	1302	1331	0.98:1
MenuButtonMinimalPerf.default	1799	1864	0.97:1
InputMinimalPerf.default	1329	1378	0.96:1
TreeWith60ListItems.default	190	216	0.88:1

Hotell · 2021-08-05T10:00:59Z

I plan to update the config to require @default instead of @DefaultValue

as briefly discussed in your previous PR, I'd say this should be discussed with vbuild - which way we wanna go ( API extractor or not api extractor for docs ? -> and with that TSDoc or JSDoc )

TSDoc has its own eslint plugin (they dont support JSDoc standard @default pragma

Hotell

going to block this for now - reasoning: should be discussed with vBuild team/broader team probably

Hotell · 2021-08-05T10:09:48Z

    'import/namespace': 'off',
    'import/no-named-as-default-member': 'off',
+
+    'jsdoc/check-tag-names': ['error', { definedTags: ['jest-environment'] }],


I'd remove that definedTags and instead turn off that rule in particular file - this is rather an edge case - a good practice is not to generalise on edge cases from my experience

Hotell · 2021-08-05T10:14:38Z

    'import/namespace': 'off',
    'import/no-named-as-default-member': 'off',
+
+    'jsdoc/check-tag-names': ['error', { definedTags: ['jest-environment'] }],


this is a topic to be discussed at vbuild sync, but I'd propose to use severity warn - lint violation is not inflicting any critical issues to runtime code - thus should not break build etc.

In general our DX is a big red squiggles ocean (for non critical violations) which is from my experience (based on feedback of big teams) very distracting.

In my experience, warnings never get fixed and just clutter up the build output. If you have warnings checked into master, then people learn to ignore warnings (and new ones get buried in the sea of warnings that other people haven't fixed). I'd say it's not worth adding this at all unless it's an error.

What @behowell said is generally what I've also said when we discussed this in the past. It seems like a fundamental philosophical difference about the purpose of linting, which I'm not sure how to resolve.

In my experience, warnings never get fixed and just clutter up the build output. If you have warnings checked into master, then people learn to ignore warnings (and new ones get buried in the sea of warnings that other people haven't fixed). I'd say it's not worth adding this at all unless it's an error.

everyone has different experience for sure :) IF you have solid CI DX (integrated into review - eg github action) and rule sets (to fail pipeline if there is a threshold reached of lint warnings), it is very nicely doable (implemented this for 100 dev org and it worked very nicely)

I'll try to prepare an RFC with setup that I'd propose going forward based on my experience in this field, where we can discuss things further. appreciate your opinions ✌️

behowell · 2021-08-05T15:51:24Z

going to block this for now - reasoning: should be discussed with vBuild team/broader team probably

@Hotell is the discussion with v-build about whether to use the jsdoc lint plugin at all? Or the specifics of whether we want @default vs. @defaultvalue tags, etc.? This PR specifically focuses on adding the jsdoc plugin without modifying existing code. We could have a follow-up discussion about the tags to use when we actually turn on the rules that cause errors, without blocking this PR.

size-auditor · 2021-08-06T17:40:07Z

Asset size changes

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

Baseline commit: 8078e6915b4e74a3e1d2d53f6ff7198fd9dc2631 (build)

fabricteam · 2021-08-06T17:44:26Z

📊 Bundle size report

Unchanged fixtures

Package & Exports	Size (minified/GZIP)
`react-accordion` Accordion (including children components)	`75.787 kB` `22.366 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	`25.016 kB` `8.035 kB`
`react-button` CompoundButton	`30.308 kB` `8.911 kB`
`react-button` MenuButton	`26.603 kB` `8.543 kB`
`react-button` ToggleButton	`34.613 kB` `8.671 kB`
`react-components` react-components: Accordion, Button, FluentProvider, Image, Menu, Popover	`177.666 kB` `50.281 kB`
`react-components` react-components: FluentProvider & webLightTheme	`36.288 kB` `11.615 kB`
`react-divider` Divider	`15.889 kB` `5.747 kB`
`react-image` Image	`10.642 kB` `4.264 kB`
`react-label` Label	`9.397 kB` `3.839 kB`
`react-link` Link	`14.715 kB` `6.012 kB`
`react-make-styles` makeStaticStyles (runtime)	`7.59 kB` `3.321 kB`
`react-make-styles` makeStyles + mergeClasses (runtime)	`22.135 kB` `8.356 kB`
`react-make-styles` makeStyles + mergeClasses (build time)	`2.557 kB` `1.202 kB`
`react-menu` Menu (including children components)	`115.15 kB` `34.665 kB`
`react-menu` Menu (including selectable components)	`117.88 kB` `35.217 kB`
`react-popover` Popover	`124.599 kB` `36.198 kB`
`react-portal` Portal	`7.78 kB` `2.672 kB`
`react-positioning` usePopper	`23.145 kB` `7.942 kB`
`react-provider` FluentProvider	`16.286 kB` `5.991 kB`
`react-text` Text - Default	`11.798 kB` `4.452 kB`
`react-text` Text - Wrappers	`15.414 kB` `4.734 kB`
`react-theme` Teams: all themes	`32.941 kB` `6.674 kB`
`react-theme` Teams: Light theme	`20.247 kB` `5.662 kB`
`react-tooltip` Tooltip	`46.054 kB` `15.658 kB`
`react-utilities` SSRProvider	`213 B` `170 B`

_{🤖 This report was generated against 8078e6915b4e74a3e1d2d53f6ff7198fd9dc2631}

Hotell · 2021-08-12T11:54:41Z

going to block this for now - reasoning: should be discussed with vBuild team/broader team probably

@Hotell is the discussion with v-build about whether to use the jsdoc lint plugin at all? Or the specifics of whether we want @default vs. @defaultvalue tags, etc.? This PR specifically focuses on adding the jsdoc plugin without modifying existing code. We could have a follow-up discussion about the tags to use when we actually turn on the rules that cause errors, without blocking this PR.

the discussion should be about

what standard do we wanna adopt -> TSDoc or JSDoc ? (which contains also the default vs defaultValue )

Based on that decision appropriate linting rules should be added

Hotell · 2021-08-12T17:03:00Z

We had additional discussion with build team today with following outcomes:

we need to create RFC with pros/cons etc for JSDoc vs TSDoc and decide which standard we wanna go forward
to unblock this PR
- please add defaultValue(NOTE its camelCase not same case - https://tsdoc.org/pages/tags/defaultvalue/)
- and remarks(https://tsdoc.org/pages/tags/remarks/) tags
  - to extend JSDoc feature set so linting will still pass and with that we temporarily support both JSDoc and TSDoc standards. once we decide which standard we wanna adhere to, we can easily update our codebase and rule set accordingly

thank you for your patience 🙌

Hotell

unblocking

…dd-eslint-plugin-jsdoc

Hotell

thank you!

ecraig12345

LGTM once the change types are fixed

…dd-eslint-plugin-jsdoc

msft-fluent-ui-bot · 2021-08-20T07:41:05Z

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

Handy links:

behowell added 5 commits August 4, 2021 18:52

Add eslint-plugin-jsdoc to check for jsdoc errors

87bdf70

Merge branch 'master' of https://github.com/microsoft/fluentui into a…

4901f94

…dd-eslint-plugin-jsdoc

Change files

cf9fc3a

Change dependentChangeType to none

efb7bfb

Delete extra change file

7fb3360

behowell requested review from Hotell and ecraig12345 August 5, 2021 03:07

behowell requested a review from a team as a code owner August 5, 2021 03:07

msft-fluent-ui-bot assigned smhigley Aug 5, 2021

behowell commented Aug 5, 2021

View reviewed changes

Hotell reviewed Aug 5, 2021

View reviewed changes

Comment thread packages/eslint-plugin/package.json Outdated

Hotell previously requested changes Aug 5, 2021

View reviewed changes

msft-fluent-ui-bot added the Needs: Author Feedback label Aug 5, 2021

msft-fluent-ui-bot removed the Needs: Author Feedback label Aug 5, 2021

Use latest version of node in devops builds

3e8f5d9

Hotell reviewed Aug 12, 2021

View reviewed changes

Hotell self-requested a review August 12, 2021 17:03

behowell added 2 commits August 12, 2021 12:15

Merge branch 'master' of https://github.com/microsoft/fluentui into a…

4a7fe17

…dd-eslint-plugin-jsdoc

Update config from feedback. Fix (new) lint errors in test files.

d182476

behowell requested a review from a team as a code owner August 12, 2021 20:22

behowell commented Aug 12, 2021

View reviewed changes

Comment thread packages/make-styles/src/renderer/createDOMRenderer-node.test.ts

Change files

dd27d52

Hotell approved these changes Aug 13, 2021

View reviewed changes

Hotell reviewed Aug 13, 2021

View reviewed changes

Comment thread .devops/templates/tools.yml

ecraig12345 approved these changes Aug 17, 2021

View reviewed changes

layershifter approved these changes Aug 17, 2021

View reviewed changes

behowell added 2 commits August 19, 2021 17:21

Update change types to none

32febbf

Merge branch 'master' of https://github.com/microsoft/fluentui into a…

b6493d3

…dd-eslint-plugin-jsdoc

behowell merged commit 21df840 into microsoft:master Aug 20, 2021

behowell mentioned this pull request Sep 16, 2021

refactor(Badge): Remove mergeProps and migrate to simple slots #19763

Merged

2 tasks

behowell deleted the add-eslint-plugin-jsdoc branch July 25, 2022 17:38

Uh oh!

Conversation

behowell commented Aug 5, 2021

Pull request checklist

Description of changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ecraig12345 Aug 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codesandbox-ci Bot commented Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fabricteam commented Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Perf Analysis (@fluentui/react-northstar)

Uh oh!

Hotell commented Aug 5, 2021

Uh oh!

Uh oh!

Hotell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

behowell commented Aug 5, 2021

Uh oh!

size-auditor Bot commented Aug 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Asset size changes

Uh oh!

fabricteam commented Aug 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📊 Bundle size report

Uh oh!

Hotell commented Aug 12, 2021

Uh oh!

Hotell commented Aug 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hotell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Hotell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ecraig12345 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

msft-fluent-ui-bot commented Aug 20, 2021

Uh oh!

Reviewers

Assignees

ecraig12345 Aug 6, 2021 •

edited

Loading

codesandbox-ci Bot commented Aug 5, 2021 •

edited

Loading

fabricteam commented Aug 5, 2021 •

edited

Loading

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

size-auditor Bot commented Aug 6, 2021 •

edited

Loading

fabricteam commented Aug 6, 2021 •

edited

Loading

Hotell commented Aug 12, 2021 •

edited

Loading