refactor: hide internal API from TS definitions #2960
Conversation
web-padawan
force-pushed
the
refactor/hide-internal-api
branch
from
ae007b9
to
dad9c16
Compare
| assertType<DelegateFocusMixin>(checkbox); | ||
| assertType<SlotLabelMixin>(checkbox); | ||
| assertType<ActiveHost>(checkbox); | ||
| assertType<DisabledHost>(checkbox); |
There was a problem hiding this comment.
DisabledHost and KeyboardHost are no longer available through ActiveHost so we have to import them explicitly and add separate assertions. All the complex mixins (FieldMixin etc) work the same.
The main change in this PR is to use classes instead of interfaces to have protected methods, and the classes can't have multiple inheritance for obvious reasons. Unfortunately, it forces us to repeat ourselves.
There was a problem hiding this comment.
UPD: DisabledMixinClass and KeyboardMixinClass are the new names, the rest of comment is still valid.
packages/combo-box/src/vaadin-combo-box-data-provider-mixin.d.ts
Outdated
Show resolved
Hide resolved
web-padawan
force-pushed
the
refactor/hide-internal-api
branch
from
99c0d7e
to
57d0aa5
Compare
| import { KeyboardMixinClass } from '@vaadin/component-base/src/keyboard-mixin.js'; | ||
| import { CheckedMixinClass } from '@vaadin/field-base/src/checked-mixin.js'; | ||
| import { DelegateFocusMixinClass } from '@vaadin/field-base/src/delegate-focus-mixin.js'; | ||
| import { LabelMixinClass } from '@vaadin/field-base/src/label-mixin.js'; | ||
| import { ThemableMixin } from '@vaadin/vaadin-themable-mixin/vaadin-themable-mixin.js'; |
There was a problem hiding this comment.
If the solution looks good, I will submit another PR to update remaining mixins e.g. ThemableMixin etc.
| @@ -31,6 +31,7 @@ | |||
| "polymer" | |||
| ], | |||
| "dependencies": { | |||
| "@open-wc/dedupe-mixin": "^1.3.0", | |||
There was a problem hiding this comment.
For now we only use Constructor type but we might want to also use dedupeMixin helper later.
It does pretty much the same thing as dedupingMixin from Polymer but in a different way.
|
Kudos, SonarCloud Quality Gate passed!
|
|
There is one drawback I could see with the proposed solution. The TypeScript doesn't report any error in case a mixin overrides a parent method with a new one which call type doesn't match the parent call type. Consider the following example: // @filename focus-mixin.d.ts
export declare const FocusMixin: <T extends Constructor<HTMLElement>>(base: T) => T & Constructor<FocusMixinClass>;
export declare class FocusMixinClass {
protected _setFocused(focused: boolean): void;
}// @filename delegate-focus-mixin.d.ts
export declare function DelegateFocusMixin<T extends Constructor<HTMLElement>>(
base: T
): T & Constructor<DelegateFocusMixinClass> & Constructor<FocusMixinClass>;
export declare class DelegateFocusMixinClass {
protected _setFocused(focused: number/* doesn't match boolean, however, there is no error */): void;
} |
There was a problem hiding this comment.
@vursen That's a good point but we can't provide a clean solution until microsoft/TypeScript#17744 is resolved.
The typeof class feature might make it possible when it lands microsoft/TypeScript#41587
The current approach is based on https://lit.dev/docs/composition/mixins/#typing-the-subclass
|
This ticket/PR has been released with platform 22.0.0.beta2 and is also targeting the upcoming stable 22.0.0 version. |
Description
Same as #2881 but using a different approach inspired by #2881 (review)
Type of change