[TS] Let's discuss how to use readonly and protected

[Reinmar]

readonly

While working on #11708 we came across an difference in how we used JSDoc's @readonly and the semantics of TS's readonly keyword.

In TS's world, a readonly property cannot be modified after being initialized in the constructor (similar to const). So far, when migrating code we were defining properties as readonly if they were @readonly in JSDoc. 

However, before the migration we allowed changing this property if the change was happening inside the class. This was sometimes used and so we were using workaround such as: https://github.com/ckeditor/ckeditor5/blob/master/packages/ckeditor5-engine/src/model/range.ts#L795-L796

We need to revisit how we port the JSDoc's @readonly from the perspective of:

• The code itself (how to migrate)
• The API docs (how the API should be used by others)

The output of this work should be:

• Clarification of when to use readonly now.
• Migration tips.
• ADR

protected

Similar to readonly – our semantics differs from the one used by TS. We need to clarify the change and come up with migration tips.

The output of this work should be:

• Clarification of when to use protected now.
• Migration tips.
• ADR

Notes

• This is a research ticket. Eventual code changes will be handled in a separate ticket.

[arkflpc]

readonly

The problem:

In JsDoc readonly tag is used as an indication, that the annotated property shouldn't be modified by the client code. The most natural representation of this in TypeScript is readonly keyword. This keyword allows changing of the property only inside the class constructor. However, we use it outside the class. Few examples:

1. Command#affectsData (link) is marked readonly, but set in constructor of derived classes
2. Plugin#isEnabled (link) is marked readonly, but updated in derived classes outside constructor
3. Properties of some immutable object are updated for performance reasons, instead of creating the objects from scratch (link)

Workarounds:

Here are some ideas of what we can do about it. We can have different solutions for different cases.

Not using readonly keywords

We can document properties as readonly without using readonly keyword. See https://tsdoc.org/pages/tags/readonly/.

class Test {
    /**
     * @readonly
     */
    public propertyName: PropertyType;
}

Pros:

• TypeScript will not complain.

Cons:

• We loose TypeScript power to force our will upon client code.

Casting to 'any'

Like in Range example.

class Test {
    public readonly propertyName: PropertyType;
}

// later...
( obj as any ).propertyName = value;

Pros:

• Always works

Cons:

• If the clients are supposed to update the property (like affectsData), forcing them to cast looks bad. It looks less bad for internal code, though.

Splitting the accessors

Getters and setters in TypeScript can have different accessibility (at least since 4.3.5).

class Test {
    private _propertyName: PropertyType;
    public get propertyName(): PropertyType { return this._propertyName; }
    private /* or protected */ set propertyName( value ) { this._propertyName = value; }
}

However, this doesn't work nice with Observable class:

class Test extends Observable {
    // We have to declare the property as public,
    // so that Observable's methods can recognize `propertyName` as `keyof Test` and have proper typing.
    //
    // We have to use `declare` keyword here, otherwise Observable#set will throw an exception (it checks,
    // whether the property exists before calling `set` the first time, so it must not exist).
    //
    // Anyway, it's not compatible with getters and setters, so we can't split it.
    declare public readonly propertyName: PropertyType;
    
    constructor() {
        this.set( 'propertyName', value );
    }
}

Pros:

• Maps the intended usage into TypeScript

Cons:

• Adds indirection to the access (performance?)
• Not compatible with observable properties

Using Observable#setto update the property

Having Test defined as above:

testObj.set( 'propertyName', value );
// instad of
// testObj.propertyName = value;

Note: Currently, Observable re-declares the property every time. One if should make Observable.set optimized.

Cons:

• Forces users to change the practice (i.e., when updating Plugin#isEnable from within custom plugin). However, only when they're moving to TypeScript, so it is backward-compatible (sort of).

[arkflpc]

 protected

The meaning of protected in JsDoc is different from protected in TypeScript. JsDocs considers it equivalent of package private in Java, not protected. And this is how we currently understand this tag. In TypeScript, it is like protected in Java. Moreover, when TypeScript parses JsDoc code, it translates JsDoc tag into a protected access modifier which changes its meaning (see TypeScript docs). It doesn't matter what the TypeScript compiler thinks because we're rewriting the code bottom-up, so .ts files don't depend on .js ones. But TypeDocs (which we decided to use), follows TypeScript meaning in that case.

TypeScript doesn't have any package private equivalent.

Two options that came to my mind:

Keep members public and annotate with internal tag

class Test {
	/** @internal */
	public method(): void { ... }
}

testObj.method();

We can not only mark it internal in API docs, but also can exclude it from generated .d.ts files using --stripInternal option.

However, skipping it from documentation introduces a small inconsistency:

// test.ts
class Test {
	/** @internal */
	public method1(): void { ... }
	private method2(): void { ... }
}

// test.d.ts
class Test {
	// It doesn't have the type, because it's an implementation detail. But it is still there, so the name
	// can't be accidentally used in derived class.
	private method2;
	
	// But... there is not method1.
}

Same of internal properties are not used outside the class, but overridden in derived ones. So, we can use protected instead of public:

class Test {
	/** @internal */
	protected method(): void { ... }
}

Cast to 'any'

We can make the property private and cast it to any if we want to use it:

class Test {
	private method(): void { ... }
}

( testObj as any ).method();

Because this is used in the same package, we don't force users to do ugly casts.

It's incompatible with overriding. We would have to stick with protected in that case.

[bczerwonka-cksource]

 protected

If you don't mind, I will add my thoughts. Have you thought about using interfaces and following "interface segregation principle"? You could operate on interfaces instead of class and thanks to this, you could have to interfaces: public and protected. Public interfaces might be exposed, and protection will be only visible in the specific package. Moreover, we had many problems in our code base if we used specific classes instead of interfaces - problems with types, mocking, etc. So we try to use interfaces everywhere.

[arkflpc]

@bczerwonka-cksource, yes, we have. We used that pattern in a few places, like Emitter.

However, in this case, the usage of this internal part is in the same file. If you have a method/function in one file (a.ts) and use it in another (b.ts), then:

1. The internal interface should probably be exported from a.ts and imported by b.ts and will be in API Docs and in a.d.ts file as well. The problem with marking them as non-public remains.
2. The class will implement an interface only when the methods are public. And public stuff will be in API Docs and .d.ts.
3. If the class in a.ts had no public methods, that should be internal, b.ts would have cast the instances (i.e., passed by parameters from the other package) to the internal interface. The cast would give us only a little more type-safety over cast to any with the cost of maintaining this interface.

[bczerwonka-cksource]

Thanks for the explanation 👍

I don't know totally how you generate docs, so probably my comments are worthless ;) However, I will share my experience/thoughts based on our project.

So, in our project, we decided to export from the module only things that can be used from other modules. For this, we decided to use `index.ts` files that contain all "public" exports - this file only re-export dependencies from the module. Of course, you still have access to specific files when you provide the correct path ... For limiting this, we use ESLint rule https://eslint.org/docs/latest/rules/no-restricted-imports 'no-restricted-imports': [ 'error', { 'patterns': [ '**/dist/*' ] } ], . It is not perfect but it is good enough for us.

Example:

file a.ts

export interface IPublic {}

export interface IInternal {}

export default class Test implements IPublic, IInternal {}

file b.ts 

import {IInternal} from './a'

export default class TestInternal implements IInternal {}

file index.ts

import { IPublic, Test } from './a'

export {
	IPublic,
	Test
}

However, in your project is a case with "protected" methods/properties that is tricky. In our project, we don't allow for it - something is public and generally visible or should be hidden in class. Probably we do not have your cases ;)  

For me, using `any` is totally bad... IMO there is no sense using TS and types when in the end, you can use `any` ;)  

Maybe using some naming convention will be good enough? In our project for private things, we use _ prefix - I know it is controversial, but we base it on our preferences, increasing the readability of code for us. So, maybe in your case, using some convention for public-protected things will be ok?

Sorry for spamming ;)

[arkflpc]

My proposal:

protected:

• Use @internal tag and keep the members public (or protected if needed).
• Stick to current coding convention (_ prefix).
• Consider omitting them from .d.ts files.

readonly:

• The property is not meant to be changed after construction, but is updated because of performance/hack: use readonly modifier and cast to any to update.
• It is possible to split getter and setter: split them and use backing field.
• Otherwise, (i.e., observable property): use @readonly tag in doclet.

[niegowski]

I feel like we should focus on the integrator/plugin developer experience so maybe we should make the API as clean as possible for such cases.

protected

• I would not mark private members/fields as public just to be accessed from automatic tests. IMHO it's not a common practice to test internals, but when it happens we always could fall back to awful cast as any.
• For the "friend" class access maybe we could introduce interfaces that would be exported from individual files but marked as internal (and stripped from .d.ts) and not exported from the main package entry file.

readonly

• for internal (engine/core) access I would use internal interface as for "friend" class explained above.
• for external usages like isEnabled, affectsData, etc. I would use a dedicated setter/getter with different access modifiers.
• for performance/hacks solutions, any seems to be ok if used only in internal engine code.

[arkflpc]

@niegowski, @bczerwonka-cksource , I agree with the interface approach. But keep in mind, that:

// a.ts
/** @internal */
export interface TestInternal {
	_fn();
}

// Can't implement TestInternal,
export class Test /* implements TestInternal */ {
	// because this is not public,
	private _fn(): void { ... }
}

// b.ts
import {Test, TestInternal } from 'a';

export function helperFunc( test: Test ) {
	// so this cast is still required here.
	( test as TestInternal )._fn();
}

Is that acceptable for you?

[bczerwonka-cksource]

// because this is not public,
private _fn(): void { ... }

🤔 Why is it not public? 

If you have an interface, it means that expected that someone will use those methods. It is a contract for others. If you create a private function, you should not add them to the interface.

I assume that _fn is a protected method. So, IMO it should look like:

// a.ts
export interface TestInternal {
	_fn();
}

export interface TestPublic {
	hello();
}

// Can't implement TestInternal,
export class Test implements TestPublic, TestInternal {
	public hello(): void {}

	public _fn(): void {}
}

export function helperFunc( test: TestInternal ) {
	test._fn();
}

// another module should use TestPublic interface only

export function hi( test: TestPublic ) {
	test.hello();
}

I don't know how you exactly use "protected" methods, but IMO code that is exposed publicly only because of testing should be hidden, and in tests, you should test everything only through public contracts. But maybe I don't know the use case.

Maybe it is a good question. When/why do you use "protected" (package private) methods/fields?

[arkflpc]

Meeting notes

Internal

Let's remove @Protected tags from members that are only exposed to tests. For other cases - change @Protected to @internal and make methods public (protected).

Readonly

If property is not meant to be changed after construction, but is updated because of performance/hack: add internal setter (may have different name or be regular method, so we can avoid indirection in getter).

Unreleted thoughts:

• Insist of documenting every use of as any

Thanks @niegowski, @bczerwonka-cksource