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

@typescript-eslint/await-thenable issue with browser.asControl<T> #509

Open
heimwege opened this issue Jul 31, 2023 · 17 comments
Open

@typescript-eslint/await-thenable issue with browser.asControl<T> #509

heimwege opened this issue Jul 31, 2023 · 17 comments
Labels
enhancement New feature or request

Comments

@heimwege
Copy link
Contributor

Describe the bug

Comparable to the TS sample I wrote:

import type List from "sap/m/List";
import type { wdi5Selector } from "wdio-ui5-service/dist/types/wdi5.types";

export default class ObjectPage extends Page {
    static async iShouldSeeListItems (numberOfItems: number) {
        const listSelector: wdi5Selector = {
            selector: {
                controlType: "sap.m.List",
                id: RegExp("FooList")
            }
        };
        const fooList = await browser.asControl<List>(listSelector);
        const fooListItems = await fooList .getItems();
        expect(fooListItems.length).toEqual(numberOfItems);
    }
}

This works in general 👍 but gets me an error at await fooList .getItems(); because method getItems of sap.m.List is not async.

ESLint: Unexpected `await` of a non-Promise (non-"Thenable") value.(@typescript-eslint/await-thenable)

But without the await the test fails.

So my question is: Is there another way to solve this than to disable the rule @typescript-eslint/await-thenable for all WDI5 tests files?

Runtime Env (please complete the following information):

  • wdi5/wdio-ui5-service-version: 1.5.1
  • UI5 version: 1.115.1
  • node-version: 16.18.0
  • OS: Win11
  • Browser + Version: chrome 115
@dominikfeininger
Copy link
Collaborator

dominikfeininger commented Aug 2, 2023

You can find working examples to retrieve an aggregation here:

const vboxItemsViaFluentApi = await browser.asControl(pageSelector).getContent(1).getItems()

and here:

const items = await oMultiComboBox.getItems()

The example contains a list in the Other.view with a test for it's items here:

return await list.getAggregation("items")

@heimwege
Copy link
Contributor Author

heimwege commented Aug 2, 2023

Thanks for the reply @dominikfeininger but I'm not sure if my question was misunderstood. The code is working fine that's not the issue. My question was related to the type check that complains that I await something that is no promise.

My test looks pretty much the same as this sample: https://github.com/ui5-community/wdi5/blob/main/examples/ui5-ts-app/test/e2e/MultiInput.test.ts

Currently I use import type List from "sap/m/List"; where getItems returns ListItemBase[] and not Promise<ListItemBase[]>.

But you linked another sample that looks differently. So I'm asking myself if the sample I used is wrong (but why would it part of the examples in the repo then)?

As the return type of browser.asControl is Promise<WDI5Control & Control> I don't see why I should not be able to type it according to the Control I expect to be able to call the methods of the control directly (sap.m.List in this case to get Promise<WDI5Control & List>).

In the meantime (and to improve my generic type skills 🐱) I experimented a bit with converting the type (in my sample sap.m.List) so that the return types of the functions are typed as promises of the original return type:

type MapFunctionToPromise<T> = T extends (...args: infer A) => infer R ? (...args: A) => Promise<R> : T;

type Promised<T> = WDI5Control & {
    [K in keyof T]: MapFunctionToPromise<T[K]>;
}

then I can do the following

const fooList = await browser.asControl<List>(listSelector) as unknown as Promised<List>;
const fooListItems = await fooList.getItems();

and get type support as now the return type of fooList.getItems() is actually Promise<ListItemBase[]>.

But if the sample I used is just wrong and I should go for the getAggregation method of the WDI5Control instead of the getItems method of sap.m.List that's of course fine as well. Then maybe the wrong sample should be removed to avoid further confusion.

@dominikfeininger
Copy link
Collaborator

Hi, yep, I misunderstood your question.

The sample is correct. Classic works on my machine ;)
In my development setup the error you mentioned does not show up.
Running MacOs with VSCode and the example repository.

Can you please double check your development setup including TS config.

@heimwege
Copy link
Contributor Author

heimwege commented Sep 1, 2023

Sorry for the delay (vacation).

Of course you don't get the error if you don't enable the respective lint check 🐱

As said everything works fine from a functional perspective. But currently I have to use UI5 types where the return type does not fit the implementation as I have to await non-Promise values. And therefore I get no support from typescript for this scenario at all. In case of the above described type transformation (to promised return types) typescript will tell you that the response type is a promise and thus prevent the issues that arise of a non awaited promises.

Sample:

const fooList = await browser.asControl<List>(listSelector) as unknown as Promised<List>; //see above described generic type
const fooListItems = fooList.getItems();
expect(commentsListItems.length).toEqual(1); //<-- type check error

will result in
TS2339: Property 'length' does not exist on type 'Promise<ListItemBase[]>'
whereas

const fooList = await browser.asControl<List>(listSelector);
const fooListItems = fooList.getItems();
expect(fooListItems.length).toEqual(1); //<-- no error

will just lead to a failed test.
Then you might look into the UI5 type and don't see any error because also there a Promise is nowhere to be seen in the return type. Then by chance you might find the solution in the WDI5 samples.

So it all comes down to the question if you are aware of this mismatch and the issues that arise out of it or if this is not seen as an issue at all from your side and has to fix on consumer side (by either disabling the respective lint check for WDI5 tests or adjusting the return type of the used UI5 types as described above).

@heimwege
Copy link
Contributor Author

Did not find a way to remove the "waiting for Input" label. Input has been provided 🤔

@vobu
Copy link
Contributor

vobu commented Sep 25, 2023

Hi Dominik, thanks for putting in the effort of that detailed description 👍
And yes, providing correct types is an issue for us. Generally speaking, we're not in a good shape when it comes to typed UI5 controls retrieved from the browser-scope. As I've tried to outline in #544 (comment), the challenging part is how to correctly type the retrieved control at runtime of the test.
Regarding your sample...

const fooList = await browser.asControl<List>(listSelector) // assuming List is of type sap.m.List
const fooListItems = await fooList.getItems() // <-- await was missing
expect(fooListItems.length).toEqual(1)

...this is the equivalent to proper typing at design-time - which works ok for most cases.
Yet there's still lots of room in improvement of the intersection type that is used behind the scenes:

asControl: <T extends Control = Control>(arg: wdi5Selector) => Promise<WDI5Control & T>

We talked about the general challenge of providing better types also in the last steering committee meetings - but frankly, no TS wizard in sight that could help 😬
Do you have a suggestion how we could improve both design-time WDI5Control and runtime .asControl typings?

@vobu
Copy link
Contributor

vobu commented Sep 25, 2023

(btw: personally: also getting proper TS checks working for the fluent async api has become kind of a nemesis for me.
sth along the lines...

await browser.asControl(selector).getVisible()

...I've yet to get working without TS constantly whining about Did you forget to use 'await'?
Gnarf.)

@LukasHeimann
Copy link
Contributor

With regards to the awaiting: For my project, I have written a complex type that takes a UI5 Control and makes all methods promise return promises. That doesn't allow for a fluent API anymore, but I was fine with that.

// Advanced types for promise-based access of UI5 methods

// Helper type to be used in the types below
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- must be any, as otherwise `extends AnyFunction` doesn't match properly
type AnyFunction = (...args: any[]) => unknown;

// Helper type that takes a function type and returns the same function type, it just returns
// a promise and wraps the return value to be recursively wrapped again
type PromisifyFunction<NonPromisedFunction extends AnyFunction> =
  (...args: Parameters<NonPromisedFunction>) => Promise<PromisifyObject<ReturnType<NonPromisedFunction>>>;

// Type that takes another type and wraps all functions as PromisedFunctions
// The idea: WDI5 gives access to all UI5 functions, but the results come back as promises
// When you wrap the UI5 type into this promised type, the WDI5-manipulated interface will be enforced
type PromisifyObject<Type> = {
  [Property in keyof Type]: Type[Property] extends AnyFunction
    ? PromisifyFunction<Type[Property]>
    : PromisifyObject<Type[Property]>
};

// The actual wrapper for WDI5 -- browser.asControl returns something that is both a WDI5 control
// as well as a proxy to UI5 methods of the control, just in a promised fashion
type WDI5<UI5Element extends Control> = WDI5Control & PromisifyObject<UI5Element>;

@vobu
Copy link
Contributor

vobu commented Sep 25, 2023

can we havez this in wdi5 core pleasez 😸
As the successor to the current WDI5Control?!
And from there, the way to a ChainablePromiseElement as wdio uses it can't be far, can it?! 😄

@github-actions
Copy link

hey 👋 - silence for 30 days 🤐 ... anybody? 😀

@github-actions github-actions bot added the stale label Oct 26, 2023
@vobu
Copy link
Contributor

vobu commented Oct 26, 2023

well, this is still a thing and hopes are still high that @LukasHeimann will get this into the core types of wdi5 😸

@vobu vobu added enhancement New feature or request and removed stale labels Oct 26, 2023
@LukasHeimann
Copy link
Contributor

I'm not sure about how this can be made chainable, honestly... I've looked at the code you linked, but unfortunately it wasn't very helpful when it comes to mapping existing types to new ones...

@FrankVisuals
Copy link

I have looked into this topic a bit and came up with the following idea.

In the browser-commands.d.ts I did the following change to test it:

asControl: <T extends Control = Control>(arg: wdi5Selector) => Promise<WDI5Control & T>;

to

asControl: <T extends Control = Control>(arg: wdi5Selector) => Promise<WDI5Control & T> & (WDI5Control & T)

While this is obviously not perfect, it will allow chaining in typescript without casts to any. Opinions on this?

@LukasHeimann
Copy link
Contributor

LukasHeimann commented Mar 27, 2024

I don't know if it is actually achievable, but ideally I would want to make sure that I must await the last call that returns a primitive value... You might want to have a mapped type that filters down to only the UI5Element-returning methods of the given type, and have it wrap their responses into the WDI5 interface recursively. All the other methods can still be kept in the recursive promisifier I've posted above:

type UI5Function = (...args: any[]) => UI5Element;
type WDI5ifyFunction<NonWdi5edFunction extends UI5Function> =
  (...args: Parameters<NonWdi5edFunction >) => WDI5<ReturnType<NonWdi5edFunction >>;

// Type that tykes a UI5 element and returns all its methods that return another UI5 element to allow chaining here
type ChainableMethods<Type extends UI5Element> = {
  [Property in keyof Type as Type[Property] extends UI5Function
    ? Property
    : never]: Type[Property] extends UI5Function ? WDI5ifyFunction<Type[Property]> : never
};

// base wdi5 methods + everything needs to be awaited recursively + methods returning a UI5 Element return WDI5<ReturnType> for chaining
type WDI5<Element extends UI5Element> = WDI5Control & PromisifyObject<Element> & ChainableMethods<Element>;

This is what I think https://www.typescriptlang.org/docs/handbook/2/mapped-types.html#:~:text=You%20can-,filter%20out%20keys,-by%20producing%20never wants me to do, but it doesn't yet work...

Edit: I think, ChainableMethods<T> on its own works just as expected, but TypeScript can't deal with the type union providing different return types (PromisifyObject<T> and WDI5<T>) for the same method, and thus only makes the former available...

@bvincent
Copy link

Hello, Do you have any update / plan to fix await-thenable issue with browser.asControl returned value ? Thanks.

@vobu
Copy link
Contributor

vobu commented Jun 25, 2024

most definitely!
and most honestly since I know this is also on the wdio^9 release agenda, we'll most likely wait for them to give us a head start 😸
and that being said: wouldn't mind if you @bvincent would give it a shot and get us ahead in the race!

@bvincent
Copy link

bvincent commented Jun 25, 2024

Hello,

I was not aware about wdio9 works in progress on that topic (see closed issues).

Thanks for the quick update,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants