Establish lint config and use across repo

[tylerbutler]

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

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

• Enable and pre-apply fixes for prefer-optional-chain #10271
• Enable prefer-nullish-coalescing eslint rule #10268
• Enable prefer-includes eslint rule and pre-apply fixes #10272
• Enable no-invalid-this ESLint rule and pre-apply fixes #10279
• unicorn/prefer-type-error
• Enable no implicit coercion eslint rule and pre-apply fixes #10315
• @typescript-eslint/prefer-ts-expect-error
• @typescript-eslint/prefer-return-this-type
• tools: Enable valid-typeof rule #10623
• require-atomic-updates
• Enable use-isnan eslint rule to base config and pre-apply fixes #10324 (already enabled via inheritance)
• unicorn/no-object-as-default-parameter
• @typescript-eslint/prefer-string-starts-ends-with
• @typescript-eslint/no-empty-function
• Enable @typescript-eslint/no-unused-vars eslint rule and pre-apply fixes #10359
• unicorn/no-lonely-if
• unicorn/no-useless-undefined
• unicorn/prefer-top-level-await
• tools: Enable unicorn/prefer-type-error rule and pre-apply fixes #10645
• unicorn/prefer-string-slice
• unicorn/prefer-string-starts-ends-with
• unicorn/prefer-string-trim-start-end
• tools: Enable unicorn/prefer-switch rule #10646
• tools: Enable unicorn/prefer-ternary rule and pre-apply fixes #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

• jsdoc/check-access: tools: Enable jsdoc/check-access and pre-fix violations #10578
• jsdoc/require-hyphen-before-param-description: tools: Enable jsdoc/require-hyphen-before-param-description #10585
• jsdoc/empty-tags (minimal)
• jsdoc/require-param-description (minimal)
• jsdoc/no-multi-asterisks (minimal)
• 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)

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

Other formatting

• Enable @typescript-eslint/member-delimiter-style and @typescript-eslint/semi #10172
• Enable space-unary-ops and switch-colon-spacing lint rules #10178
• Enable tsdoc/syntax lint rule and pre-apply fixes #10144

Phase 1

• Change comma-dangle lint rule to always-multiline #10087
• Remove eslint-plugin-no-null #10261
• Enable tsdoc/syntax lint rule and pre-apply fixes #10144

Other

• Reenable eslint rule @typescript-eslint/unbound-method #9082
• Finalize eslint ruleset for repo #9499
• strictNullChecks should be true for all projects #9500
• Enable react eslint plugin #10010
• Add eslint-plugin-jsdoc to allow enforcement of source code docs style, and potentially enforce docs presence in the future #10317
• Make sure all our packages use our shared lint and tsconfig #10351

Notes

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

[Josmithr]

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

[tylerbutler]

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.

[Josmithr]

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.

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

[Josmithr]

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

[tylerbutler]

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

[Josmithr]

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

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

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

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

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.

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