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

[TS Followup] Change mixins style #12077

Closed
arkflpc opened this issue Jul 18, 2022 · 2 comments · Fixed by #12140
Closed

[TS Followup] Change mixins style #12077

arkflpc opened this issue Jul 18, 2022 · 2 comments · Fixed by #12140
Assignees
Labels
domain:ts squad:core Issue to be handled by the Core team.

Comments

@arkflpc
Copy link
Contributor

arkflpc commented Jul 18, 2022

There are two styles of mixins suggested by TypeScript Handbook: suggested and alternative one.

Currently, the usage of mixins in ckeditor5 looks like:

export default class MyClass { ... }
mix(MyClass, EmitterMixin);

After migration to TypeScript, we could use the alternative pattern and translate the above into:

class MyClass { ... }
mix(MyClass, EmitterMixin);
interface MyClass extends Emitter {}
export default MyClass;

However, when we want to mix both DomEmitterMixin and ObservableMixin:

class MyClass { ... }
mix(MyClass, DomEmitterMixin);
mix(MyClass, ObservableMixin);

// won't work, because listenTo must be identical in all super-interfaces:
// interface MyClass extends DomEmitter, Observable {}

// But type intersection works differently, so let's use that:
type ObservableDomEmitter = DomEmitter & Observable;
interface MyClass extends ObservableDomEmitter {}
export default MyClass;

This is doable, but seems like a hack. I propose to use the pattern from handbook:

export default class MyClass extends ObservableMixin( DomEmitterMixin( Object ) ) { ... }

This could be made backward-compatible (XyzMixin functions would have all the methods also exposed on the function object itself, at least for the existing mixins).

@arkflpc arkflpc added squad:core Issue to be handled by the Core team. domain:ts labels Jul 18, 2022
@arkflpc
Copy link
Contributor Author

arkflpc commented Jul 19, 2022

Check if tests are not noticeably slower.

@CKEditorBot CKEditorBot added status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. and removed status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. labels Jul 19, 2022
@arkflpc arkflpc self-assigned this Jul 25, 2022
@arkflpc
Copy link
Contributor Author

arkflpc commented Jul 27, 2022

Add more info in ChangeLog (i.e. It's now recommended to use ... ).

arkflpc added a commit that referenced this issue Jul 28, 2022
Other (utils): Introduces new way to apply mixins. Instead of using mix() util, classes should extend the base created by EmitterMixin(), ObservableMixin() and DomEmitterMixin() or extend Emitter or Observable directly. Closes #12077.
@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Jul 28, 2022
@CKEditorBot CKEditorBot added this to the iteration 56 milestone Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:ts squad:core Issue to be handled by the Core team.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants