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

.isA() should be a type guard #387

Closed
sap-sebelao opened this issue Feb 23, 2023 · 5 comments
Closed

.isA() should be a type guard #387

sap-sebelao opened this issue Feb 23, 2023 · 5 comments
Assignees
Labels
enhancement New feature or request legacy ts-types (with globals) Related to legacy Global types (deprecated) types ES modules types (recommended)

Comments

@sap-sebelao
Copy link
Member

sap-sebelao commented Feb 23, 2023

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 #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
Copy link
Member

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

akudev commented Feb 27, 2023

@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
Copy link
Member Author

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

akudev commented Feb 28, 2023

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 akudev self-assigned this Mar 3, 2023
@akudev akudev added enhancement New feature or request legacy ts-types (with globals) Related to legacy Global types (deprecated) types ES modules types (recommended) labels Mar 3, 2023
openui5bot pushed a commit to SAP/openui5 that referenced this issue Mar 3, 2023
Fixes SAP/ui5-typescript#387

Change-Id: I9af434c901b991dfe425f364e6e94997bc17bc6e
@akudev
Copy link
Contributor

akudev commented Mar 3, 2023

Fixed by SAP/openui5@481bc37 (both the static and the instance variant on sap.ui.base.Object) and will be in the 1.112 types.
Thanks!

@akudev akudev closed this as completed Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request legacy ts-types (with globals) Related to legacy Global types (deprecated) types ES modules types (recommended)
Projects
None yet
Development

No branches or pull requests

3 participants