-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
fix(cdk/testing): strongly type return value of TestElement.getProperty #22918
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -206,8 +206,8 @@ export class ProtractorElement implements TestElement { | |||
} | |||
|
|||
/** Gets the value of a property of an element. */ | |||
async getProperty(name: string): Promise<any> { | |||
return browser.executeScript(`return arguments[0][arguments[1]]`, this.element, name); | |||
async getProperty<T = any>(name: string): Promise<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ideally we'd make the default unknown
but I guess maybe that's too breaking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I suspect that unknown
will be breaking. I doubt that getProperty
is used too often since most of the time you can get the same information through getAttribute
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but we should see if we can change this to unknown
for v13
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Agreed on unknown
being better in the future; we could have a migration for that.
8751e89
to
1b80053
Compare
async getProperty(name: string): Promise<any> { | ||
return browser.executeScript(`return arguments[0][arguments[1]]`, this.element, name); | ||
async getProperty<T = any>(name: string): Promise<T> { | ||
return browser.executeScript<T>(`return arguments[0][arguments[1]]`, this.element, name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems that google's internal typings do not support a generic version of executeScript
can you change this to return browser.executeScript(...) as any
Allows for the return value of `TestElement.getProperty` to be typed strongly through a generic parameter.
Done. Also rebased. |
1b80053
to
87ec42a
Compare
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Allows for the return value of
TestElement.getProperty
to be typed strongly through a generic parameter.