ts: enable noImplicitOverride

[paul-marechal]

This feature helps with quickly identifying overridden methods, as well
as notify us of potential breakage whenever APIs are removed and some
classes still try to override those missing methods.

It seems like this change wouldn't even force dependents to update their TS version as the override keyword doesn't make its way into the generated .d.ts files.

How to test

Try to remove one of the override and build: it should fail.

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

[paul-marechal]

Doing this made me notice how many readonly we have, I don't actually think those provide much value in most cases...

Worth a PR to remove those too? I'd be in favor.

[colin-grant-work]

Doing this made me notice how many readonly we have, I don't actually think those provide much value in most cases...

Worth a PR to remove those too? I'd be in favor.

I think the readonly annotations are or can be useful, and since it would require though to decide which is which, it isn't worth a wholesale change. The should mainly be used to indicate things that should not be reassigned. A lot of injected services (where we have most of our readonlys) fall into that category: reassigning them would potentially break event emission and disposal very badly. In most cases, people aren't going to be tempted to reassign them, so the readonly isn't do a lot of work, but it's doing some.

[JonasHelming]

How have you created these override annotations? Was there an automated way?

[paul-marechal]

@JonasHelming I didn't find an automated way to do it than ctrl-clicking on TS build errors and pasting the missing keywords. I see this as a small time investment to set this all up :)

[paul-marechal]

@colin-grant-work I think we could wholesale remove the readonly from all injected properties.

For one for property injection to work something needs to write from the exterior into those fields. So far Inversify is doing it because it doesn't care about TS readonly (and can't). I just find this modifier to be noisy in this case. Moreover it should be common practice to not just re-assign protected fields without knowing what you are doing.

[colin-grant-work]

...it should be common practice to not just re-assign protected fields without knowing what you are doing.

Many things are good practice that we nevertheless ask our tools to help us do :-).

[colin-grant-work]

Looks good to me. I checked the cases where you've deleted injections, and they all look correct. One potential hanging comment, but if that's intentional, then it's good to go.

[paul-marechal]

Many things are good practice that we nevertheless ask our tools to help us do :-).

@colin-grant-work Agreed, just like what is being done in this PR :)

The difference I would make is that readonly semantics depend on our implementation's design, whereas override is systematic (if missing/present when it should/shouldn't we get an error).

And I don't like our uses of readonly as it feels like it got used systematically rather than trying to properly design our APIs...

[colin-grant-work]

And I don't like our uses of readonly as it feels like it got used mechanically rather than trying to properly design our APIs...

I don't disagree with you about the substance of your claim, but you may take my argument as a warning that a PR removing a lot of readonly's probably isn't a good idea, because it would require a lot more care on the part of the reviewer than a PR like this one, which is almost purely mechanical, for not much benefit (but perhaps not no benefit), so you might not find eager reviewers.

[paul-marechal]

@colin-grant-work @JonasHelming I added a new commit that cleans up the code base even more based on the redundancy of fields as highlighted by this new override check.

I noted the few niche constructor signature breakage in the changelog.

[vince-fugnitto]

The changes look good to me 👍 CI is also successful (build, tests)

[msujew]

The changes are looking good to me as well 👍

[colin-grant-work]

I'll add my endorsement as well. 👍