Convert to use TS2.2 Class Mixins over dojo/compose #326
Conversation
Codecov Report@@ Coverage Diff @@
## master #326 +/- ##
=========================================
- Coverage 98.78% 97.28% -1.5%
=========================================
Files 16 14 -2
Lines 656 663 +7
Branches 110 107 -3
=========================================
- Hits 648 645 -3
- Misses 1 2 +1
- Partials 7 16 +9
Continue to review full report at Codecov.
|
agubler
changed the title
agubler
changed the title
|
The i18n-specific changes look good. 👍 |
|
Custom element stuff looks good 👍 |
| /** | ||
| * Register handles for the instance that will be destroyed when `this.destroy` is called | ||
| * | ||
| * @param {Handle} handle The handle to add for the instance |
There was a problem hiding this comment.
Nit: remove types from comments as per guidelines
There was a problem hiding this comment.
Add a note that we need to follow up on tsdoc throughout the repo.
src/mixins/ProjectorMixin.ts
Outdated
| readonly projectorState: ProjectorState; | ||
| } | ||
|
|
||
| export function ProjectorMixin<T extends WidgetConstructor>(base: T): T & Constructor<Projector> { |
There was a problem hiding this comment.
Any reason this isn't the default export? Same goes for WidgetBase I think.
There was a problem hiding this comment.
I have done this for all classes, it's a topic for discussion i guess but I you'll rarely just use the default.
| /** | ||
| * WidgetBase constructor type | ||
| */ | ||
| export type WidgetConstructor = new (...args: any[]) => WidgetBase<WidgetProperties>; |
There was a problem hiding this comment.
Would it make sense to write this as
export type WidgetConstructor = Constructor<WidgetBase<WidgetProperties>>;?
There was a problem hiding this comment.
I think that absolutely makes sense @maier49, do you mind if we fix the up in a follow up PR?
Type: feature
The following has been addressed in the PR:
Description:
Convert widget-core to use TS2.2 class mixins instead of
dojo/composeThere are a couple of interesting changes included
baseThemeproperty for the Themeable mixin is set via a class decorator (@themeexported from Themeable)class MyWidget extends ThemeableMixin(WidgetBase)<MyWidgetProperties>and excluding this does actually provide a pretty obscure errorNo base constructor has the specified number of type arguments.along with a few others.const Projector = ProjectorMixin(Button);Items to follow up on:
Resolves #327