Skip to content

[V8] Updated props to optional for getText and getLocation commands#12307

Merged
christian-bromann merged 7 commits intowebdriverio:v8from
jemishgopani:optional-props-doc
Feb 20, 2024
Merged

[V8] Updated props to optional for getText and getLocation commands#12307
christian-bromann merged 7 commits intowebdriverio:v8from
jemishgopani:optional-props-doc

Conversation

@jemishgopani
Copy link
Copy Markdown
Contributor

Proposed changes

Fixes #12173

  • Make props as optional parameter for getText and getLocation commands

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

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)

Reviewers: @webdriverio/project-committers

@christian-bromann
Copy link
Copy Markdown
Member

Thanks for the contribution! Can you raise the same PR for main branch as well?

@jemishgopani
Copy link
Copy Markdown
Contributor Author

Thanks for the contribution! Can you raise the same PR for main branch as well?

Sure

@jemishgopani jemishgopani changed the title Updated props to optional for getText and getLocation commands [V8] Updated props to optional for getText and getLocation commands Feb 18, 2024
@jemishgopani
Copy link
Copy Markdown
Contributor Author

jemishgopani commented Feb 19, 2024

@christian-bromann I have added tests as well can you please take a look again? if all things are good then I'll add this same changes in other PR.

Copy link
Copy Markdown
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.

Some question, it seems like this bug wasn't validated in the first place as given changes don't actually let the test fail.

this: WebdriverIO.Element,
prop?: keyof Location
): Promise<Location | number> {
): Promise<any> {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't see the need to make this any, what is the reasoning for this change?

Suggested change
): Promise<any> {
): Promise<Location | number> {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

if here we use return type as Location | number then the declaration refrence is not working as expected for getLocation() it will ask for an 1 argument in the warning because reference for IDE always point to export function getSize (this: WebdriverIO.Element, prop: keyof RectReturn): Promise<number> declaration.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am not sure if this is the right solution then. We shouldn't return any as we loose type information about the return value. I might be wrong here though, do you have any reference documentation that recommends this approach?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yup you are correct that is not right, I have updated the code with a different solution

this: WebdriverIO.Element,
prop?: keyof RectReturn
): Promise<Size | number> {
): Promise<any> {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

same

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same answer as above

@jemishgopani jemishgopani marked this pull request as draft February 20, 2024 03:57
@jemishgopani jemishgopani marked this pull request as ready for review February 20, 2024 08:53
Copy link
Copy Markdown
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.

Awesome, LGTM 👍

@christian-bromann christian-bromann added the PR: Bug Fix 🐛 PRs that contain bug fixes label Feb 20, 2024
@christian-bromann christian-bromann merged commit 423f030 into webdriverio:v8 Feb 20, 2024
@wdio-bot
Copy link
Copy Markdown
Contributor

Hey jemishgopani 👋

Thank you for your contribution to WebdriverIO! Your pull request has been marked as an "Expensable" contribution. We've sent you an email with further instructions on how to claim your expenses from our development fund. Please make sure to check your spam folder as well. If you have any questions, feel free to reach out to us at expense@webdriver.io or in the contributing channel on Discord.

We are looking forward to more contributions from you in the future 🙌

Have a nice day,
The WebdriverIO Team 🤖

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Expensable $25 💸 PR: Bug Fix 🐛 PRs that contain bug fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants