Use strict eslint config in azure-service-utils #9489
Conversation
github-actions
bot
added
the
area: framework
| @@ -36,7 +36,8 @@ | |||
| }, | |||
| "dependencies": { | |||
| "@fluidframework/protocol-definitions": "^0.1027.1000", | |||
| "jsrsasign": "^10.2.0" | |||
| "jsrsasign": "^10.2.0", | |||
| "uuid": "^8.3.1" | |||
There was a problem hiding this comment.
This dependency was completely missing. I think the only reason it has been building OK is because of lerna dependency hoisting.
| if (documentId === undefined) { | ||
| throw new Error("documentId cannot be undefined"); | ||
| } |
There was a problem hiding this comment.
I don't understand why this function has a nullable documentId parameter... Could I make it required? Or use a different type for the claims?
| @@ -4,7 +4,7 @@ | |||
| "src/test/**/*" | |||
| ], | |||
| "compilerOptions": { | |||
| "strictNullChecks": false, | |||
| "strictNullChecks": true, | |||
There was a problem hiding this comment.
Scary that this was disabled...
| @@ -5,13 +5,20 @@ | |||
|
|
|||
| module.exports = { | |||
| "extends": [ | |||
| "@fluidframework/eslint-config-fluid" | |||
| "@fluidframework/eslint-config-fluid/strict" | |||
There was a problem hiding this comment.
I see what you're doing here, permitting piecemeal improvements to the ruleset. However I think an opt-in approach is slower than an opt-out approach and makes it easy for new packages to continue adopting bad habits (in much the same way that having our rules disabled for a period of time permitted decay of our code quality).
I'd suggest getting a single, strict ruleset established in the eslint package and turn it on for all packages at the same time. For packages that don't comply, use disables in their individual eslintrc as necessary. This gets the most rules enabled for the most packages in the shortest period of time, and ensures any new package is strictly enforced by default. Cleaning out eslintrc's across the repo is then just a burndown effort.
There was a problem hiding this comment.
Good advice, thanks. #9501 tracks all the relevant work I know of -- take a look and let me know if there's something missing.
ensures any new package is strictly enforced by default.
Minor quibble -- because we don't have a proper project template or scaffold in place, people copy/paste projects, which means project-level overrides tend to follow new projects around like zombies. :)
There was a problem hiding this comment.
That's true, but diligent reviewers should object to new packages being added with gross disables during PR (same as they should object to incorrect README text, etc.).
There was a problem hiding this comment.
diligent reviewers
Who are these people you speak of? 😝
|
Closing without merging since this review was just to collect feedback. |
No description provided.