Feedback: ESLint ruleset #9502

tylerbutler · 2022-03-16T00:41:17Z

I'm not sure how to best review this, so this PR is an attempt. If you have other ideas, they are welcome.

I exported the complete config (using eslint --print-config) that applies to a TypeScript file in a project using our strict config. That was you can comment on specific rules. However, it's still tough to know what all the rules do. :( I don't have any good solutions, but below are links to the docs for the big rulesets/plugins we're using so rules are easier to look up.

There is also a second file, changes-from-minimal-to-strict.json, that is a diff of today's minimal config (used my 99% of packages) and the new strict config.

The following PRs contain examples of the practical effects of applying this config to some of our packages.

common/build/eslint-config-fluid/strict-ts.json

tylerbutler · 2022-03-16T18:34:26Z

common/build/eslint-config-fluid/strict-ts.json

+    "@typescript-eslint/no-parameter-properties": [
+      "warn",
+      {
+        "allows": [
+          "private",
+          "private readonly",
+          "public readonly",
+          "readonly"
+        ]
+      }
+    ],


This prevents properties from being declared within the parameters of constructors unless they're private or readonly.

This will probably be super disruptive to fix up, and I personally don't mind this TS language feature. And what's the reasoning behind the specific restrictions here (only allow private/readonly)?

Declaring parameters in constructors is foreign to non-TS devs, but more importantly, they make it more difficult to find where class-level properties are declared when reading code. It's a variant of the "declare as close to use as possible" rule -- "declare all class members together."

But I agree that cleanup may be ugly. And it does require more code.

And it does require more code

At least the emitted JS is the same, it appears, based on a quick check in TS Playground.

If there were a rule to require each such param on its own line in the constructor declaration I'd definitely be in favor of that, I'm strict to do that myself.

common/build/eslint-config-fluid/strict-ts.json

Closes microsoft#9683: Duration in "MsnStatistics" event is tracking garbage Helps better understand microsoft#9682: MSN window is too large! This change adds - tracking of collab window at summarization, to better understand impact on quality of summary (search, size of catchup blobs). - tracking of batch sizes, time to inbound and msn size at that time - I expect to see msn size correlating with batch sizes, as collab window grows to at least the size of batch at the moment when batch is sent - Correct tracking of how long it takes for a sequence number to fall out of collab window.

Correctly handle the case where multiple containers upload the same blob and submit blobAttach ops with identical blob IDs due to blob de-duping. In this case, BlobManager should only resolve the deferred promise when it sees its own op.

tylerbutler · 2022-04-05T22:55:37Z

This is now covered by #9740 and #9748, so I'm closing this.

…o require explicit modifiers (#13134) Updates rule to match discussion outcome in [this discussion](#9502 (comment)). Rule documentation: https://typescript-eslint.io/rules/explicit-member-accessibility/ Also fixes violations in packages using the `strict` config ### Breaking Change Note that this is a breaking change with respect to the above rule. We are changing error-level enforcement of the policy to the opposite rule, so code changes will be required. This PR pre-fixes the violations existing at this point in time, but any changes between now and when the new package is published and adopted will need to be fixed.

tylerbutler mentioned this pull request Mar 16, 2022

Finalize eslint ruleset for repo #9499

Closed

8 tasks

tylerbutler commented Mar 16, 2022

View reviewed changes