FluidFramework Establish lint config and use across repo

Our lint config used through most of the repo is insufficient. We need to establish a baseline config that is stricter to maintain overall code quality in the repo.

The plan

For each of the rules to be enabled, we'll make (a) a PR that enables the rule in the shared config and pre-applies fixes for the rule. Once that is merged, then (b) a pre-release shared config package is released, and then we can (c) update the version of the shared config used in the repo.

Step (a) might happen several times before (b) or (c), effectively 'batching' the config changes when it comes time to apply them to the repo.

This approach requires more bookkeeping but helps keep each individual PR focused and makes them easier to review, because all the code changes in a PR should be similar. Also, because each PR is a single rule, it's easy to have "should this rule even be turned on?" discussions without completely derailing the review.

Rules not yet done are listed first so they're easier to find. They're listed in the order we'll create PRs, and the order will be adjusted as needed.

If a rule you want to enabled is missing from the list, leave a comment. If you want to see a rule enabled sooner, leave a comment. If you have any feedback about the plan, leave a comment. :smile:

Note on the unicorn rules: they are mostly from the unicorn/recommended config.

Remaining rules

See https://github.com/microsoft/FluidFramework/wiki/ESLint for how to create a PR to enable any of the rules below.

Formatting only

[ ] unicorn/number-literal-case

Code-impacting

[x] #10271
[ ] #10268
[x] #10272
[x] #10279
[x] unicorn/prefer-type-error
[x] #10315
[ ] @typescript-eslint/prefer-ts-expect-error
[ ] @typescript-eslint/prefer-return-this-type
[x] #10623
[ ] require-atomic-updates
[x] ~~#10324~~ (already enabled via inheritance)
[ ] unicorn/no-object-as-default-parameter
[ ] @typescript-eslint/prefer-string-starts-ends-with
[ ] @typescript-eslint/no-empty-function
[x] #10359
[ ] unicorn/no-lonely-if
[ ] unicorn/no-useless-undefined
[ ] unicorn/prefer-top-level-await
[ ] #10645
[ ] unicorn/prefer-string-slice
[ ] unicorn/prefer-string-starts-ends-with
[ ] unicorn/prefer-string-trim-start-end
[x] #10646
[x] #10661
[ ] unicorn/prefer-spread
[ ] unicorn/prefer-array-find
[ ] unicorn/prefer-at
[ ] unicorn/prefer-code-point
[ ] unicorn/prefer-date-now
[ ] unicorn/prefer-array-flat
[ ] unicorn/prefer-array-flat-map
[ ] unicorn/prefer-array-index-of
[ ] unicorn/prefer-array-some
[ ] unicorn/catch-error-name
[ ] unicorn/consistent-destructuring
[ ] unicorn/no-array-callback-reference
[ ] unicorn/import-style
[ ] unicorn/new-for-builtins
[ ] unicorn/no-abusive-eslint-disable
[ ] unicorn/no-array-callback-reference
[ ] unicorn/no-array-for-each
[ ] unicorn/no-array-method-this-argument
[ ] unicorn/no-array-push-push
[ ] unicorn/no-array-reduce
[ ] unicorn/no-await-expression-member
[ ] unicorn/no-console-spaces
[ ] unicorn/no-document-cookie
[ ] unicorn/no-empty-file
[ ] unicorn/no-for-loop
[ ] unicorn/no-hex-escape
[ ] unicorn/no-instanceof-array
[ ] unicorn/no-invalid-remove-event-listener
[ ] unicorn/no-keyword-prefix
[ ] unicorn/no-new-array
[ ] unicorn/no-new-buffer
[ ] unicorn/no-object-as-default-parameter
[ ] unicorn/no-process-exit
[ ] unicorn/no-static-only-class
[ ] unicorn/no-thenable
[ ] unicorn/no-this-assignment
[ ] unicorn/no-unreadable-array-destructuring
[ ] unicorn/no-useless-fallback-in-spread,
[ ] unicorn/no-useless-length-check
[ ] unicorn/no-useless-promise-resolve-reject
[ ] unicorn/no-useless-spread
[ ] unicorn/no-useless-undefined
[ ] unicorn/no-zero-fractions
[ ] unicorn/number-literal-case
[ ] unicorn/prefer-add-event-listener
[ ] unicorn/prefer-default-parameters
[ ] unicorn/prefer-dom-node-append
[ ] unicorn/prefer-dom-node-dataset
[ ] unicorn/prefer-dom-node-remove
[ ] unicorn/prefer-dom-node-text-content
[ ] unicorn/prefer-export-from
[ ] unicorn/prefer-json-parse-buffer
[ ] unicorn/prefer-keyboard-event-key
[ ] unicorn/prefer-math-trunc
[ ] unicorn/prefer-modern-dom-apis
[ ] unicorn/prefer-module
[ ] unicorn/prefer-negative-index
[ ] unicorn/prefer-number-properties
[ ] unicorn/prefer-object-from-entries
[ ] unicorn/prefer-optional-catch-binding
[ ] unicorn/prefer-prototype-methods
[ ] unicorn/prefer-query-selector
[ ] unicorn/prefer-reflect-apply
[ ] unicorn/prefer-regexp-test
[ ] unicorn/prefer-set-has
[ ] unicorn/relative-url-style
[ ] unicorn/require-array-join-separator
[ ] unicorn/require-number-to-fixed-digits-argument

Documentation

[x] jsdoc/check-access: #10578
[x] jsdoc/require-hyphen-before-param-description: #10585
[x] jsdoc/empty-tags (minimal)
[x] jsdoc/require-param-description (minimal)
[ ] jsdoc/no-multi-asterisks (minimal)
[x] jsdoc/no-bad-blocks (minimal)
[ ] jsdoc/require-description (recommended)
[ ] jsdoc/require-description-complete-sentence (strict)
[ ] jsdoc/check-alignment (minimal)
[ ] jsdoc/check-param-names (recommended)
[ ] jsdoc#/empty-tags (minimal)
- We will also want to specify additional tags to cover TSDoc modifier tags not covered by the base rule. See here for rule instructions.
[ ] jsdoc/newline-after-description (recommended)
[ ] jsdoc/require-jsdoc (strict)
- With publicOnly set to true.
[ ] jsdoc-require-param-name (minimal)
[ ] jsdoc/require-param (strict)
[ ] jsdoc/require-returns-check (recommended)

Strict

These rules are very strict and may make sense to enable on a package-by-package basis, rather than repo-wide like with the other rules. For that reason, I suggest we enable these in a separate strict config. I'm leaving them until last because applying them will be time consuming.

[ ] @typescript-eslint/explicit-function-return-type
[ ] @typescript-eslint/explicit-member-accessibility
[ ] @typescript-eslint/explicit-module-boundary-types
[ ] @typescript-eslint/no-unsafe-argument
[ ] @typescript-eslint/no-unsafe-assignment
[ ] @typescript-eslint/no-unsafe-call
[ ] @typescript-eslint/no-unsafe-member-access

Done

Formatting round 1 (#10056)

[x] @typescript-eslint/brace-style
[x] @typescript-eslint/comma-spacing
[x] @typescript-eslint/func-call-spacing
[x] @typescript-eslint/keyword-spacing
[x] @typescript-eslint/object-curly-spacing
[x] @typescript-eslint/space-before-function-paren
[x] @typescript-eslint/space-infix-ops
[x] @typescript-eslint/type-annotation-spacing
[x] array-bracket-spacing
[x] arrow-spacing
[x] block-spacing
[x] key-spacing

Other formatting

[x] #10172
[x] #10178
[x] #10144

Phase 1

[x] #10087
[x] #10261
[x] #10144

Other

[x] #9082
[x] #9499
[ ] #9500
[ ] #10010
[x] #10317
[ ] #10351

Notes

If possible, avoid project level overrides. Prefer line-level overrides, then block-level, then file-level.

Mar 15 '22 23:03 tylerbutler

Personally, I would be in favor of adding no-implicit-coercion to the list as well, as I think it greatly enhances readability.

May 16 '22 20:05 Josmithr

Personally, I would be in favor of adding no-implicit-coercion to the list as well, as I think it greatly enhances readability.

Nice! I didn't know about that rule. I added it towards the top of the list, but I'd like to also do a check to see if we have a ton of violations in the current code. I need to write down how to do that in the wiki, so this is a good opportunity to document it as I go.

May 17 '22 16:05 tylerbutler

I'm also curious what folks' opinions are on adding the "ignoreUrls": true exception to our max line-length rule, for the cases where we add links to our code docs. Not really a new rule per se, but it came to mind as I was working in the codebase.

May 19 '22 00:05 Josmithr

@tylerbutler What do you think about adding this rule to our strict config? https://www.npmjs.com/package/eslint-plugin-jsdoc#user-content-eslint-plugin-jsdoc-rules-require-description

Would require that tsdoc comments always include the description field. For generated API docs in particular, I think this would be useful to enforce.

Jun 07 '22 21:06 Josmithr

I was also going to suggest multiline-blocks with noSingleLineBlocks and noZeroLineText. Consitent formatting enhances readability. Probably add to recommended rather than minimal?

Jun 09 '22 22:06 Josmithr

@Josmithr I love these additions. I think we should chat about the minimal/recommended/strict configs and how we envision them being used long-term. @ChumpChief has made good points in the past of the value of having a single config that is quite strict. If that's our end-goal, we might change how we stage the changes. I'll spin up a separate conversation about this.

Would require that tsdoc comments always include the description field. For generated API docs in particular, I think this would be useful to enforce.

I do like the intention here, but I want to be careful to not reward "useless docs" that meet the 'requirements' but don't help developers. I.e. I don't want to incentivize people to write bad docs just to make the linter happy.

That said, that's no reason not to turn on the rule. Rather, we should monitor the quality of the docs and adjust accordingly.

Jun 13 '22 19:06 tylerbutler

@Josmithr Joshua Smithrud (HE/HIM) FTE I love these additions. I think we should chat about the minimal/recommended/strict configs and how we envision them being used long-term. @ChumpChief Matt Rakow FTE has made good points in the past of the value of having a single config that is quite strict. If that's our end-goal, we might change how we stage the changes. I'll spin up a separate conversation about this.

Would require that tsdoc comments always include the description field. For generated API docs in particular, I think this would be useful to enforce.

I do like the intention here, but I want to be careful to not reward "useless docs" that meet the 'requirements' but don't help developers. I.e. I don't want to incentivize people to write bad docs just to make the linter happy.

That said, that's no reason not to turn on the rule. Rather, we should monitor the quality of the docs and adjust accordingly.

100% agree. I see the linter rule as a nudge to the dev to think about the need to write docs. We need to leverage best practices and code reviews to ensure what is produced is quality.

Jun 13 '22 20:06 Josmithr

I think we may want to reconsider unicorn/prefer-negative-index - it reduces code size a bit, but I think the result is much less immediately understandable.

Sep 26 '22 20:09 Josmithr

I think we may want to reconsider unicorn/prefer-negative-index - it reduces code size a bit, but I think the result is much less immediately understandable.

Hmm, this may be something that I like because Python has it, and it fits my mental model better. I could go either way.

Nov 29 '22 23:11 tylerbutler

I think we may want to reconsider unicorn/prefer-negative-index - it reduces code size a bit, but I think the result is much less immediately understandable.

Hmm, this may be something that I like because Python has it, and it fits my mental model better. I could go either way.

It could definitely be that I am in the minority for this too, and my opinion on this one isn't a super strong. Maybe worth getting more opinions on.

Dec 12 '22 19:12 Josmithr

Another rule worth considering: https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/consistent-type-imports.md

Preferring type imports whenever possible will likely help us reduce bundle sizes pretty effectively, and I think it's a good habit for devs to get into.

Dec 12 '22 19:12 Josmithr