Update client monorepo to eslint 7 #3770
Conversation
tylerbutler
force-pushed
the
eslint/client
branch
2 times, most recently
from
3e80cd1 to
67277f0
Compare
|
I recognize it's difficult to review so many changes in a single PR, so I am open to suggestions for how to make this easier to review without breaking the monorepo build. |
| @@ -11,7 +11,9 @@ interface IWindow extends Window { | |||
| // Extract the head and body HTML. Remove any <script> tag. And then wrap inside a basic HTML page. | |||
| export function createCacheHTML(): void { | |||
| const [bodyHTML, headHTML] = [document.body.innerHTML, document.head.innerHTML]; | |||
| // eslint-disable-next-line unicorn/no-unsafe-regex | |||
There was a problem hiding this comment.
This seems scary - potentially exponential-time, according to this? https://github.com/sindresorhus/eslint-plugin-unicorn/blob/master/docs/rules/no-unsafe-regex.md
| @@ -37,7 +37,8 @@ export class FluidSerializer implements IFluidSerializer { | |||
| // eslint-disable-next-line @typescript-eslint/strict-boolean-expressions | |||
| const handle = !!value && value.IFluidHandle; | |||
| // TODO - understand why handle === false in some of our tests | |||
| // eslint-disable-next-line @typescript-eslint/strict-boolean-expressions | |||
| // eslint-disable-next-line max-len | |||
There was a problem hiding this comment.
🎂 I only reviewed the .ts and .tsx files. All looks pretty reasonable.
I did leave some comments, take a look. And to add to your list from the initial comment, I'm curious about the module-related ones, like no-var-requires etc. I don't know much about how that works and what goes bad with the different syntax options.
|
/azp run "Build - client" |
|
No pipelines are associated with this pull request. |
|
/azp run "repo-policy-check" |
|
No pipelines are associated with this pull request. |
tylerbutler
force-pushed
the
eslint/client
branch
from
e05e22f to
aa2f3d8
Compare
|
/azp run "Build - client" |
|
No pipelines are associated with this pull request. |
|
🎉 |
This PR upgrades the client monorepo to ESLint 7. During the course of doing this, I noted a few rules that are worth discussing more.
@typescript-eslint/strict-boolean-expressions
This rules requires
strictNullChecks=truein tsconfig in order to be useful. Many of our packages don't have this enabled (for various reasons). But they really should, no? Anyway, the docs indicate that the rule is useless without this config, so it's ignored in a lot of projects.@typescript-eslint/restrict-template-expressions
We violate this a lot, but I didn't notice anything terrible in the cases I spot checked. My recommendation is we leave this disabled.
@typescript-eslint/no-unsafe-return
We could improve type-safety by enabling this rule and fixing what it finds. But the violations are numerous...
@typescript-eslint/ban-types
We use the
object,{}, andFunctiontypes quite a bit, so we aren't getting as much type safety as we could be. From the docs:Avoid the Function type, as it provides little safety for the following reasons:
Avoid the Object and {} types, as they mean "any non-nullish value".
Avoid the object type, as it is currently hard to use due to not being able to assert that keys exist.
These do seem worth fixing at some point, but I'm not sure what we should replace
Objectwith.