enable TS strict mode

[paul-marechal]

What it does

Enable TS strict checks and fixes related problems in the code base.

I used a few strategies:

• Any injected field is marked as not null with !
• Unless there is a @optional() decorator in which case I mark the field as nullable with ?
• Initialize boolean values with false because undefined is falsy (to conserve the current logic)
• Initialize some fields as early as possible by slightly changing the logic
• When not possible to initialize fields, I added checks at use time to fail early if something is undefined when it shouldn't

Then there's another special case with some classes like:

@injectable()
export class SomeOptions {
  readonly option1: string
  readonly option2: number
}

My guess is that this was more convenient than writing:

export const SomeOptions = Symbol('SomeOptions');
export interface SomeOptions {
  readonly option1: string
  readonly option2: number
}

The issue is that in the class version, the field are declared as not nullable, but they aren't initialized either. This means that doing new SomeOptions() will give back a bogus object. Furthermore those classes aren't ever instantiated: they're always used as a type and some regular JSON object is passed instead. To keep as much semantic, I fixed the issue by making both the class and its non nullable fields abstract.

Alternatively I could convert those classes into symbol+interface if we think it's a better idea.

Close #10417

How to test

Pure refactoring, everything should keep working like before.

Review checklist

• As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• As a reviewer, I agree to behave in accordance with the review guidelines

[colin-grant-work]

First round...

[colin-grant-work]

How much do we gain if we mark it potentially undefined, and at the only mention that matters, simply assert that it isn't undefined?

[paul-marechal]

My guess is that if we had the strict checks enabled before, this method wouldn't have been implemented with this oddity in the first place. So not sure what to think about this code indeed. What I like right now is that it properly describes how root can be undefined but that this place expects it not to be.

[msujew]

In the rare case that this could become undefined in the future, I believe that having some sort of guard for that would be great. Something as simple as if (!this.root) { return; }

[colin-grant-work]

Why remove this default?

[paul-marechal]

I had understood that BackendApplicationCliContribution.configure/setArguments wasn't called when this component was used, but turns out it might because the CLI is initialized in the generated app. Will revert this part now that I better understand how it works. I got confused.

[JonasHelming]

@paul-marechal : Are you ready to trigger a re-review from colin?

[paul-marechal]

@JonasHelming I will need to rebase first. I'd also like to have other opinions about this. I believe this is for the better despite the additional strictness, but I know some prefer it more lax...

[JonasHelming]

@paul-marechal : I think we should not let this go stale, could you bring it up in the dev call again, please?

[paul-marechal]

@JonasHelming if #10355 gets merged, there's even one more flag I'd like to turn on which would cause yet another big PR to align the code base with it: https://www.typescriptlang.org/tsconfig#noImplicitOverride

I'll bring it up at the next dev meeting.

[paul-marechal]

So many conflicts... I'll close for now and come back to enable this flag in bits later.