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

Second+ level "metadata" inheritance issues #338

Closed
iljapostnovs opened this issue Feb 12, 2022 · 10 comments
Closed

Second+ level "metadata" inheritance issues #338

iljapostnovs opened this issue Feb 12, 2022 · 10 comments
Assignees
Labels
ts-interface-generator Related to the ts-interface-generator sub-package

Comments

@iljapostnovs
Copy link

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!

@akudev
Copy link
Contributor

akudev commented Mar 11, 2022

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
Copy link
Contributor

akudev commented Mar 11, 2022

We would benefit from microsoft/TypeScript#33892 here.

@iljapostnovs
Copy link
Author

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 microsoft/TypeScript#33892 gets resolved, I don't mind.

@akudev akudev added the ts-interface-generator Related to the ts-interface-generator sub-package label Mar 22, 2022
akudev added a commit that referenced this issue Apr 4, 2022
@akudev
Copy link
Contributor

akudev commented Apr 4, 2022

@Revest117
Copy link

Revest117 commented Jul 1, 2022

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"
	}
}

@akudev
Copy link
Contributor

akudev commented Nov 16, 2022

@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;
}

openui5bot pushed a commit to SAP/openui5 that referenced this issue Nov 28, 2022
...in type names. Needed in particular for the newly to-be-introduced
sap.ui.base.Object.$ObjectMetadata etc.

Related to SAP/ui5-typescript#338

Change-Id: I70c77de47ec5bd75373b04214d8401d7a272aff5
@akudev
Copy link
Contributor

akudev commented Nov 29, 2022

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).

codeworrior added a commit to SAP/ui5-builder that referenced this issue Dec 19, 2022
...in type names. Needed in particular for the newly to-be-introduced
sap.ui.base.Object.$ObjectMetadata etc.

Related to SAP/ui5-typescript#338

Cherry picked from SAP/openui5@7deccb5c7.
openui5bot pushed a commit to SAP/openui5 that referenced this issue Dec 19, 2022
Related to SAP/ui5-typescript#338

Change-Id: Ida726a9ced5a9c322694576f9c4d73dd5609e2a0
codeworrior added a commit to SAP/ui5-builder that referenced this issue Dec 19, 2022
Contains the following changes:

* [INTERNAL] JSDoc: do not show a module for namespaces without exports

When a namespace is not exported by a module, don't show a module for
the namespace in the SDK. This prevents useless module information for
purely virtual namespaces like sap.ui.base, sap.ui.model.json etc.

Cherry picked from SAP/openui5@396262641.

* [INTERNAL] jsdoc publish.js: type parser: accept underscores and $ signs

...in type names. Needed in particular for the newly to-be-introduced
sap.ui.base.Object.$ObjectMetadata etc.

Related to SAP/ui5-typescript#338

Cherry picked from SAP/openui5@7deccb5c7.

* [INTERNAL] JSDoc publish.js: write type of @typedefs to api.json

as "extends" - when the @typedef has properties and is not a type alias
or function prototype

Cherry picked from SAP/openui5@5c496b23d.

* [INTERNAL] JSDoc: do not write 'module' info for borrowed APIs (part 2)

This is a follow-up to SAP/openui5@9d3fccd91) which only handled the
'module' and 'export' properties and only for borrowed methods.
The same logic now applies to the 'resource' property and also to
borrowed properties (fields) and events.

Cherry picked from SAP/openui5@d6215d715.
@akudev
Copy link
Contributor

akudev commented Dec 20, 2022

Ok, with SAP/openui5@8f935aa 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 added a commit to SAP-samples/ui5-typescript-control-library that referenced this issue May 26, 2023
akudev added a commit to SAP-samples/ui5-typescript-helloworld that referenced this issue May 26, 2023
- Make use of MetadataOptions type
- Update to ts-interface-generator 0.6.2
- Update to UI5 1.114.0
- Update to TypeScript 5.0

Related: SAP/ui5-typescript#338
akudev added a commit to akudev/generator-ui5-ts-library that referenced this issue May 26, 2023
- Since 1.110.0, the "MetadataOptions" type allows full typing of the
control metadata. Also, specifying this type - or just "object" below
1.110.0 - enables inheriting from such controls (cf.
SAP/ui5-typescript#338). Both is done by this
change.

- ts-interface-generator has been extended with JSDoc generation; update
to this version.

- Also .gitignore any content in the "test" folder (which houses
generated trial projects)
akudev added a commit to SAP-samples/ui5-typescript-tutorial that referenced this issue May 26, 2023
@akudev
Copy link
Contributor

akudev commented May 26, 2023

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 akudev closed this as completed May 26, 2023
@iljapostnovs
Copy link
Author

iljapostnovs commented May 26, 2023

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ts-interface-generator Related to the ts-interface-generator sub-package
Projects
None yet
Development

No branches or pull requests

3 participants