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

docs(typings): #6458 use a more suitable return type for waitUntil #6574

Conversation

iamkenos
Copy link
Contributor

@iamkenos iamkenos commented Mar 10, 2021

Proposed changes

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update - typings

Checklist

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • I have added proper type definitions for new commands (if appropriate)

Further comments

Reviewers: @webdriverio/project-committers

@iamkenos iamkenos force-pushed the docs/6458-return-type-for-waitUntil branch 2 times, most recently from cec23d1 to 9780313 Compare March 10, 2021 03:36
Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

waitUntil returns everything that will be successfully resolved by the promise from the condition function (not only true). Therefor I assume it should be any | void.

@iamkenos
Copy link
Contributor Author

waitUntil returns everything that will be successfully resolved by the promise from the condition function (not only true). Therefor I assume it should be any | void.

Thanks for checking @christian-bromann !

Not quite sure I follow though...

The jsdocs say * @return {Boolean} true if condition is fulfilled, also the condition function should always return a truthy value and the check for that is already imposed by the argument type. From there, if the resolved value isnt true, throw an error. That's how I understood it atleast...

Can i request for an example where it would return a value other than true please?

@christian-bromann
Copy link
Member

Can i request for an example where it would return a value other than true please?

Sure, I think the comments are wrong then, I double checked:

 browser.waitUntil(() => 'foobar')
'foobar'
 browser.waitUntil(() => 123)
123

@iamkenos
Copy link
Contributor Author

Can i request for an example where it would return a value other than true please?

Sure, I think the comments are wrong then, I double checked:

 browser.waitUntil(() => 'foobar')
'foobar'
 browser.waitUntil(() => 123)
123

i see, thanks!

this is actually what i meant by - ...the check for that is already imposed by the argument type.

image

one thing we can do is change the arg type to truthy instead of boolean, then combine it with generics to use the same arg type as the return type. it's not gonna be straightforward but it will be more accurate.

personally, i've mixed feelings about this...thoughts?

@@ -50,7 +50,7 @@ export default function waitUntil(
interval = this.options.waitforInterval,
timeoutMsg
}: Partial<WaitUntilOptions> = {}
) {
): true | void {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
): true | void {
): Promise<true | void> {

@christian-bromann
Copy link
Member

one thing we can do is change the arg type to truthy instead of boolean, then combine it with generics to use the same arg type as the return type. it's not gonna be straightforward but it will be more accurate.

That would be my preference given that this is how it actually works, regardless of the types. Also I can see that user would like to use the value returned by waitUntil

@iamkenos
Copy link
Contributor Author

one thing we can do is change the arg type to truthy instead of boolean, then combine it with generics to use the same arg type as the return type. it's not gonna be straightforward but it will be more accurate.

That would be my preference given that this is how it actually works, regardless of the types. Also I can see that user would like to use the value returned by waitUntil

Noted, thanks! Lemme see what i can do!

@iamkenos iamkenos force-pushed the docs/6458-return-type-for-waitUntil branch from 3b13e91 to 6f44c2b Compare March 15, 2021 04:35
@iamkenos
Copy link
Contributor Author

one thing we can do is change the arg type to truthy instead of boolean, then combine it with generics to use the same arg type as the return type. it's not gonna be straightforward but it will be more accurate.

That would be my preference given that this is how it actually works, regardless of the types. Also I can see that user would like to use the value returned by waitUntil

Noted, thanks! Lemme see what i can do!

hi @christian-bromann

i tried this approach and bumped into the limitation that was highlighted -- Note how it won't allow anything which it thinks might be falsy...

samples:

browser.waitUntil(() => '') // type error, expected
browser.waitUntil(() => 123) // ok
browser.waitUntil(() => 'false') //ok
browser.waitUntil(() => true) // ok
browser.waitUntil(() => false) // type error, expected
browser.waitUntil(() => el.isClickable()) // type error, should be allowed but might be falsy so nope

for now i kept the changes minimal, as per last requests. thoughts?

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 thank you so much!

This was referenced Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Polish 💅 PRs that contain improvements on existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stricter type definition for waitUntil
2 participants