Omit readonly keyword on injected fields?

[paul-marechal]

Looking at https://github.com/eclipse-theia/theia/wiki/Coding-Guidelines there isn't any guideline for injected field definition in classes.

I propose to get rid of readonly modifiers on injected field for the following reasons:

1. Since we use dependency injection, those fields are meant to be written into from the outside to inject said dependencies.
2. It makes testing easier as it avoids having to cast the instance to any or anything else to write to the field.
3. It makes the codebase lighter, the modifier just adds noise in my opinion.

[tsmaeder]

I would disagree with this: read-only fields are not supposed to change after initialisation. This makes it easier to argue about state transitions of an object. That we work around this for testing, etc. does not make the point invalid, IMO.

[paul-marechal]

I really think this is just noise. Point in case: method fields are writable and this never caused an issue. See snippet.

interface MyImmutableService {
    readonly fieldA: object
    readonly fieldB: object
    someMethod(): void
    otherMethod(): void
}

declare const service: MyImmutableService

service.fieldA = {} // error as expected
service.someMethod = () => {} // TS is fine here!

Furthermore if you look at field definitions in the framework we often don't bother to make them readonly. See search results.

Having said that, I don't want to completely get rid of the readonly keyword. Public interfaces may still define readonly data. Maybe some internal field would benefit from the modifier (I doubt for common cases).

[paul-marechal]

If anything, no matter what the consensus turns out to be I'd like for the codebase to be more consistent. Either we use readonly everywhere it makes sense, or we get rid of it in places where it adds noise.

[kkistm]

I agree with @tsmaeder that readonly makes easier to argue about the state transitions of an object.
But I have another proposition. Maybe its time to change to constructor arguments injection instead of properties injection? InversifyJS documentation recommends to use constructor injections.
Also the properties to keep injected values could be kept readonly, and there will be no problems with unit tests.