openui5 icon indicating copy to clipboard operation
openui5 copied to clipboard

Interfaces as string literals in `sap/ui/core/library`

Open pubmikeb opened this issue 1 year ago • 4 comments

In https://github.com/SAP/openui5/commit/b7d11ca8f5f9bf0e465eca018ed5804157320949, the usage of IAsyncContentCreation interface has been hardcoded as a string literals instead of using corresponding property of sap/ui/core/library:

Before:

sap.ui.define([
	"sap/ui/core/library"
], function(library) {
	return UIComponent.extend("sap.ui.demo.toolpageapp.Component", {
		metadata: {
			manifest: "json",
			interfaces: [library.IAsyncContentCreation]
		},
	}
}

After:

sap.ui.define([
], function() {
	return UIComponent.extend("sap.ui.demo.toolpageapp.Component", {
		metadata: {
			manifest: "json",
			interfaces: ["sap.ui.core.IAsyncContentCreation"]
		},
	}
}

I always assumed, that, due to possible typos and the following codebase maintenance, it's not a good idea to hardcode something as a plain text, which can be defined with the programming language.

So, several questions:

  1. Could you, please, elaborate what are the reason for replacing sap/ui/core/library#IAsyncContentCreation with hardcoded sap.ui.core.IAsyncContentCreation?

  2. Should I follow the same change in my codebase?

  3. Does it mean that sap/ui/core/library is going to be deprecated / replaced by something else?

pubmikeb avatar Nov 10 '23 09:11 pubmikeb

Reg. 1: the reasoning is the same as in https://github.com/SAP/ui5-typescript/issues/386 . The (long time existing) API design of UI5 in this case does not translate well into the TypeScript world. We expose two aspects of interfaces under the same name in UI5:

  1. the interface as a type itself. This aspect is translated to TypeScript interfaces today
  2. the string that is used to represent the interface in UI5 runtime metadata. This does not appear in TypeScript

At runtime, no conflict exists as aspect 1) has no runtime relevance. But in TypeScript, both aspects would conflict and we decided to omit the string aspect, as the type seems more important to us.

Reg. 2: not yet, maybe. Once we have an update for the best practices for UI5 1.x, this either will document it as a proposed change or not (depending on internal discussions).

Reg. 3: no, not at all.

codeworrior avatar Nov 10 '23 09:11 codeworrior

Regarding:

the string that is used to represent the interface in UI5 runtime metadata. This does not appear in TypeScript

Perhaps, it's possible to define kind of enums for such strings, which represent the interfaces? This allows to avoid the hardcoded interfaces in the codebase.

pubmikeb avatar Nov 10 '23 10:11 pubmikeb

Thank you for sharing this finding. I've created an internal incident 2370149229. The status of the issue will be updated here in GitHub.

flovogt avatar Nov 15 '23 14:11 flovogt

Hi @pubmikeb,

so we already discussed this topic shortly before the holidays, but will need to pick it up at a later point in time. Right now we don't have a timeline for this, but there are other TypeScript related issues that we will look into too.

I collected the information from this issue into a backlog item for the team: CPOUI5FRAMEWORK-582

Best regards,

Thorsten

Thodd avatar Jan 10 '24 11:01 Thodd