SAP / ui5-typescript

Tooling to enable TypeScript support in SAPUI5/OpenUI5 projects
https://sap.github.io/ui5-typescript
Apache License 2.0
200 stars 28 forks source link

.isA() should be a type guard #387

Closed sap-sebelao closed 1 year ago

sap-sebelao commented 1 year ago

When using methods such as byId, which have a very generic return type, TS then throws errors unless there is a type assertion (this topic was brought up in https://github.com/SAP/ui5-typescript/issues/356)

const comboBox = view.byId("myCombobox"); // byId returns type UI5Element; would need "as ComboBox" to narrow type
comboBox.getSelectedKey(); // TS shows error

Using type assertions is not a great approach, as it does not ensure the type will be correct on runtime. In this case, using the built in .isA method can help:

const comboBox = view.byId("myCombobox");
if(comboBox.isA("sap.m.ComboBox")) {
    comboBox.getSelectedKey(); // still shows error
}

However, TS still throws errors, because it does not recognize isA as a type guard.

This could be fixed by generics, which in this case would make sense in my opinion:

isA<T>(name: string): this is T

Usage:

if(control.isA<ComboBox>("sap.m.ComboBox")) { ... }

In general, the expression <variable> is <type> in place of return type makes TS understand the method/function as a type guard. In this case, replacing the variable with this and a specific type with the T generic makes TS understand that this method is checking that the type of this is T.

Of course, it partially relies on the developer to provide the correct/matching types for both the Type and the Name argument (but since it is an actual runtime check, such errors should get caught in testing immediately).

I think this would greatly benefit the usage of TS with UI5 as it would eliminate most needs for explicit type assertions.

I tested the above with a few simple situations with classes, interfaces and inheritance, and found no issues with this so far, but of course there could be limitations when put into practice inside a larger framework like Open/SAPUI5.

Could you consider this addition?

codeworrior commented 1 year ago

To me this sounds like a useful and rather simple addition to the UI5 type signatures.

The only unfortunate thing is the necessary redundancy in the call (type param + string param). But the benefit of the type guard for the after-following block looks greater than that drawback.

akudev commented 1 year ago

@sap-sebelao What do you mean with "existing usage of isA in TS code would become invalid"?

Existing uses seem to work as before even without the overload that re-adds the non-generics signature. Anything special you had in mind here?

sap-sebelao commented 1 year ago

@sap-sebelao What do you mean with "existing usage of isA in TS code would become invalid"?

Existing uses seem to work as before even without the overload that re-adds the non-generics signature. Anything special you had in mind here?

I checked again and you are right: I think I assumed TS will show the same error as if you do not provide a parameter when you are asserting a generic type, eg. as Record is an error, you need as Record<string, string>

It does not seem to be the case for functions however! My bad, I will edit the post to avoid confusion.

akudev commented 1 year ago

No problem, I might have assumed the same. I mainly asked because internally the implementation is easier for changing than for adding (due to how we overlay TypeScript-specific info).

akudev commented 1 year ago

Fixed by https://github.com/SAP/openui5/commit/481bc376f0d79e5d17eec627f88d6a0455e4d0e0 (both the static and the instance variant on sap.ui.base.Object) and will be in the 1.112 types. Thanks!