ui5-typescript icon indicating copy to clipboard operation
ui5-typescript copied to clipboard

Second+ level "metadata" inheritance issues

Open iljapostnovs opened this issue 3 years ago • 5 comments

There are conflicts with extending the class which extends standard UI5 class. Example: MyControl extends Control, contains test property OneMoreControl extends MyControl, contains anotherTest property image

OneMoreControl gets error: Class static side 'typeof OneMoreControl' incorrectly extends base class static side 'typeof MyControl'.\n The types of 'metadata.properties' are incompatible between these types.\n Property 'test' is missing in type '{ anotherTest: string; }' but required in type '{ test: { type: string; }; }'.

There are two ways of dealing with it:

  1. Adding properties to OneMoreControl from MyControl, but I guess it would fail at runtime, because, if I recall correctly, metadata field is removed at runtime by UI5 framework.
  2. Adding @ts-expect-error

Both options seems to be bad. Any suggestions? Thanks!

iljapostnovs avatar Feb 12 '22 21:02 iljapostnovs

As a workaround at least typing the metadata field in the first-level TypeScript control as e.g. object like this:

static readonly metadata: object = {

works and might be more elegant than the two suggestions so far, see this sandbox example.

Of course rather than object it could be a type (defined within the UI5 type definitions?) that is actually describing the metadata structure in detail (I drafted the beginning of one here). I don't think we define such a type now, but it might anyway be helpful.

I have to admit that at first glance I do not understand why it is not sufficient to do this type assignment in the base class "ManagedObject", but it has to be done in the direct superclass. Probably because the typing itself of the field is not inherited, it "only" has to conform.

I'd love to find a solution which avoids extra things to be added to the TS control code, but for someone facing this issue right now, this might be a viable workaround.

akudev avatar Mar 11 '22 16:03 akudev

We would benefit from https://github.com/microsoft/TypeScript/issues/33892 here.

akudev avatar Mar 11 '22 17:03 akudev

Hi @akudev, Thanks for the possible workaround idea! Indeed, creating the proper type for metadata field seems to be the best solution so far.

I think it would be good idea to document this limitation somewhere.

I got my answer and this issue can be closed, however, if you want to leave it open before https://github.com/microsoft/TypeScript/issues/33892 gets resolved, I don't mind.

iljapostnovs avatar Mar 12 '22 16:03 iljapostnovs

The problem is now mentioned at https://github.com/SAP/ui5-typescript/blob/main/packages/ts-interface-generator/README.md#second-level-inheritance

akudev avatar Apr 04 '22 15:04 akudev

Hi @akudev,

I interfaced the metadata, from what I could find is usable in it and starting from the sap.ui.base.ManagedObject class:

export interface DnDMetadata {
	droppable: boolean,
	draggable: boolean
}

export interface ForwardingMetadata {
	idSuffix?: string,
	getter: string,
	aggregation: string,
	forwardBinding?: boolean
}

export interface AggregationMetadata {
	type: string,
	multiple?: boolean,
	singularName?: string,
	visibility?: "hidden" | "public",
	bindable?: boolean | "bindable",
	forwarding?: ForwardingMetadata,
	selector?: string,
	dnd?: boolean | DnDMetadata,
	group?: "Accessibility" | "Appearance" | "Behavior" | "Data" | "Designtime" | "Dimension" | "Identification" | "Misc"
}

export interface AssociationMetadata {
	type: string,
	multiple?: boolean,
	singularName?: string,
	visibility?: "hidden" | "public",
	group?: "Accessibility" | "Appearance" | "Behavior" | "Data" | "Designtime" | "Dimension" | "Identification" | "Misc"
}

export interface PropertyMetadata {
	type: string,
	visibility?: "hidden" | "public"
	byValue?: boolean
	group?: "Accessibility" | "Appearance" | "Behavior" | "Data" | "Designtime" | "Dimension" | "Identification" | "Misc"
	defaultValue?: unknown,
	selector?: string,
	bindable?: boolean | "bindable"
}

export interface EventParamterMetadata {
	type: string
}

export interface EventMetadata {
	allowPreventDefault?: boolean
	parameters?: Record<string, EventParamterMetadata | string>
}

export interface MetadataObjectMetadata {
	library?: string,
	defaultProperty ?: string,
	defaultAggregation?: string,
	properties?: Record<string, PropertyMetadata | string>,
	events?: Record<string, EventMetadata>,
	aggregations?: Record<string, AggregationMetadata | string>,
	associations?: Record<string, AssociationMetadata | string>,
	specialSettings?: Record<string, unknown>,
	interfaces?: string[],
	publicMethods?: string[],
	abstract?: boolean,
	final?: boolean,
	dnd?: boolean | DnDMetadata,
	deprecated?: boolean
}

It helps a lot to write the metadata quickly and correctly. But the interfaces for the metadata would still have to be adapted so that also the classes sap.ui.base.ManagedObject is based of are considered.

Example:

static readonly metadata: MetadataObjectMetadata = {
	properties: {
		test: "string"
	}
}

Revest117 avatar Jul 01 '22 15:07 Revest117

@Revest117 thanks for your preparation work.

Despite the lack of reaction here, we haven't ignored this topic, but the implementation is tricky: you know, the type definitions of UI5 are generated from the JSDoc, so the proper way would be to specify these types in JSDoc and let the generator do its job. This would allow maintaining the types together with the code, which would help with keeping both consistent with each other.

However, I haven't found a good way to do this so far. Problems include (IIRC):

  • one cannot mark @member/ @var in a JSDoc @interface as optional
  • for UI5 interfaces we generate marker properties like __implements__sap_ui_base_EventProviderMetadataObject: boolean; which we do not want here
  • using @class instead of @interface causes constructors to be present
  • using @typedef instead of @interface means we cannot use inheritance, so we have to define the same properties in the deriving types again, causing redundancy, risk of inconsistencies, and more effort
  • (not sure about this one:) the [key: string]: any way of allowing any additional properties together with well-defined known ones cannot be expressed in JSDoc (and IIIRC we need this)

Of course we could just write the whole thing in TypeScript and add it during the generation step. But this would mean maintaining the documentation of the structure twice: In JSDoc fulltext for the SDK documentation and in TypeScript with proper typing of the objects. Sooner or later they would differ.

Anyway, from my perspective, the full set of standard metadata object definitions on all relevant levels up to "Control" would look like as below.
However, this is not completely final, as there are:

  • some properties still missing, which are not documented, but used a lot ("designtime": 196 times in SAPUI5)
  • some properties still missing, which are not documented and very rarely used ("baseType" in mdc's AdaptationProvider, "views" in sap.uxap.BlockBase, "defaultProperty" in CodeEditor and three mdc controls etc. - they might be by mistake, just like the camelCase "designTime" used in three very different controls.
  • deriving classes which introduce more properties (Component has "version", "includes", dependencies", "config", "customizing", the webc controls have "tag" and sometimes "getters", the viz controls have "vizChartType" and ControllerExtension has "methods")

The naming is still open, the parameter is called "ClassInfo" in the documentation. But the simpler *Metadata names you chose won't work because there are already classes like "ElementMetadata" etc. in UI5 and using them twice would cause confusion. So either -MetadataObject or -ClassInfo, I would say. Maybe the latter, because the former sounds funny in some combinations.

This is just to sum up the current state.

export interface BaseObjectMetadataObject {
	interfaces?: string[];
	publicMethods?: string[];
	abstract?: boolean;
	final?: boolean;
	// TODO: used, but not documented in Object.js:
	deprecated?: boolean;
}

export interface ManagedObjectMetadataObject extends BaseObjectMetadataObject {
	library?: string;
	properties?: Record<string, PropertyMetadataObject | string>;
	defaultProperty?: string;
	aggregations?: Record<string, AggregationMetadataObject | string>;
	defaultAggregation?: string;
	associations?: Record<string, AssociationMetadataObject | string>;
	events?: Record<string, EventMetadataObject>;
	specialSettings?: Record<string, unknown>;
}

export interface ElementMetadataObject extends ManagedObjectMetadataObject {
	dnd?: boolean | DnDMetadataObject;
}

export interface ControlMetadataObject extends ElementMetadataObject {
	// nothing added
}

export interface PropertyMetadataObject {
	type: string | string[];
	visibility?: "hidden" | "public"; // default: "public"
	byValue?: boolean;
	group?: "Accessibility" | "Appearance" | "Behavior" | "Data" | "Designtime" | "Dimension" | "Identification" | "Misc";
	defaultValue?: unknown;
	bindable?: boolean | "bindable";
	selector?: string;
}

export interface AggregationMetadataObject {
	type: string;
	multiple?: boolean;
	singularName?: string;
	visibility?: "hidden" | "public"; // default: "public"
	bindable?: boolean | "bindable";
	forwarding?: ForwardingMetadataObject;
	selector?: string;
	// TODO: the following is only available for aggregations from Element or Control
	dnd?: boolean | DnDAggregationMetadataObject;
	// TODO: the following is not documented in the SDK
	group?: "Accessibility" | "Appearance" | "Behavior" | "Data" | "Designtime" | "Dimension" | "Identification" | "Misc";
}

export interface AssociationMetadataObject {
	type: string;
	multiple?: boolean;
	singularName?: string;
	visibility?: "hidden" | "public";
	// TODO: the following is not documented in the SDK
	group?: "Accessibility" | "Appearance" | "Behavior" | "Data" | "Designtime" | "Dimension" | "Identification" | "Misc";
}

export interface EventMetadataObject {
	allowPreventDefault?: boolean;
	parameters?: Record<string, EventParameterMetadataObject | string>;
}

export interface EventParameterMetadataObject {
	type: string;
}

export interface DnDMetadataObject {
	droppable?: boolean;
	draggable?: boolean;
}

export interface DnDAggregationMetadataObject extends DnDMetadataObject{
	layout: "Vertical" | "Horizontal"; // default: "Vertical"
}

export interface ForwardingMetadataObject {
	idSuffix?: string;
	getter?: string;
	aggregation: string;
	forwardBinding?: boolean;
}

akudev avatar Nov 16 '22 09:11 akudev

FYI: we have enriched the JSDoc processing to enable specifying inheritance in @typedef. So we are going to express the above types directly in the UI5 source code and have them generated to d.ts (and displayed as types on their own in the documentation).

akudev avatar Nov 29 '22 08:11 akudev

Ok, with https://github.com/SAP/openui5/commit/8f935aa5d277291bbb985db5aebb76546637873e the type definitions for the "metadata" objects have landed. They can already be seen at and around https://openui5nightly.hana.ondemand.com/api/sap.ui.core.Element.MetadataOptions. They will be released with version 1.110 of the type definitions and once this happened (~mid January) I will adapt the TS control dev examples/tutorials to use them - and close this issue.

akudev avatar Dec 20 '22 09:12 akudev

Using the MetadataOptions type for the metadata (or just "object" for versions below 1.110.0) solves this issue. All related samples have been updated (or have at least a PR to do so). Closing.

akudev avatar May 26 '23 14:05 akudev

Using the MetadataOptions type for the metadata (or just "object" for versions below 1.110.0) solves this issue. All related samples have been updated (or have at least a PR to do so). Closing.

Thank you a lot!

iljapostnovs avatar May 26 '23 15:05 iljapostnovs