Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

jsdoc: core>Command replace readonly with protected #10332

Closed
wants to merge 1 commit into from
Closed

jsdoc: core>Command replace readonly with protected #10332

wants to merge 1 commit into from

Conversation

fedemp
Copy link
Contributor

@fedemp fedemp commented Aug 9, 2021

After reading the comment at #504 (comment) about properties that are marked as readonly when they should be writable, I realized that maybe you people meant protected when you used readonly here. Protected properties are writable but should only be modified by its own parent, not from the outside e.g. isEnabled that should be modified by refresh().

After reading the comment at #504 (comment) about properties that are marked as `readonly` when they should be writable, I realized that what you people meant `protected` when you used `readonly` here. _Protected_ properties are writable but should be modified by its own parent, not from the outside e.g. `isEnabled` that should be modified by `refresh()`.
@Reinmar
Copy link
Member

Reinmar commented Aug 17, 2021

According to https://stackoverflow.com/questions/36843357/typescript-difference-between-private-and-protected-variables, these 2 props are not protected as they can be accessed from other classes. 

We (try to) follow these rules: https://ckeditor.com/docs/ckeditor5/latest/framework/guides/contributing/code-style.html#accessibility

In addition to that, we understand "readonly" as "it can be changed by the class itself, but not by the outside world". So @public/protected/private define visibility, while @readonly defines who can change it. 

I'm afraid that the semantic that we apply to these terms is different than what TS defined. In this case, when generating typings from our JSDoc comments, I'd suggest stripping information that's incompatible. Changing the meaning of these symbols across CKE5's entire code base is something that would need to be done thoroughly and would thus be a bigger task.

@fedemp
Copy link
Contributor Author

fedemp commented Aug 17, 2021

No need to change ckeditor source then. But I would like to make it clear how TS should understand this.

Properties marked as readonly in ckeditor are expected to be modified only by the class and its subclasses, right?

At the same time, these properties can be read from the outside, right too?

If those two assumptions are fine, then the best typing would be like

    get foo():string;
    protected set foo(newvalue: string);

Screenshot from 2021-08-17 11-10-11

@Reinmar
Copy link
Member

Reinmar commented Aug 17, 2021

Properties marked as readonly in ckeditor are expected to be modified only by the class and its subclasses, right?

Yes, that's correct.

At the same time, these properties can be read from the outside, right too?

Yes.

If those two assumptions are fine, then the best typing would be like

As far as I understand, yes :) @arkflpc, could you confirm?

@arkflpc
Copy link
Contributor

arkflpc commented Aug 18, 2021

Yes, that's correct. The properties shouldn't be changed by Command clients but by derived classes. However, JSDoc accessibility attributes relate to the whole symbol, not setters and getters. That's why it's read-only.

@fedemp
Copy link
Contributor Author

fedemp commented Aug 18, 2021

Excellent. I'll close this PR and i'll start doing a quick check on our typings for those readonly that should actually have a protected setter.

@fedemp fedemp closed this Aug 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants