-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
chore(types): Inherit from webdriver.WebDriver types #4016
Conversation
@@ -811,7 +738,7 @@ export class ProtractorBrowser extends AbstractExtendedWebDriver { | |||
* @returns {!webdriver.promise.Promise} A promise that will be resolved to | |||
* the located {@link webdriver.WebElement}. | |||
*/ | |||
findElement(locator: Locator): WebElement { | |||
findElement(locator: Locator): WebElementPromise { |
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.
This should have been WebElementPromise
the whole time
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.
We should fix the JSDoc @returns
to reflect the same return type.
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.
Done
@@ -811,7 +738,7 @@ export class ProtractorBrowser extends AbstractExtendedWebDriver { | |||
* @returns {!webdriver.promise.Promise} A promise that will be resolved to | |||
* the located {@link webdriver.WebElement}. | |||
*/ | |||
findElement(locator: Locator): WebElement { | |||
findElement(locator: Locator): WebElementPromise { |
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.
We should fix the JSDoc @returns
to reflect the same return type.
I decided to address this comment: angular#4000 (comment) While doing do I decided to take on this TODO: https://github.com/angular/protractor/blob/ccf02ab5f1070f0d7124318dc0099252f3c747e2/lib/browser.ts#L38 One possible issue here is that `ProtractorBrowser` only copies over methods, so it doesn't actually implement `WebDriver`'s interface. This isn't a problem right now, since `WebDriver`'s interface only has functions on it. But it could be a problem in the future.
I decided to address this comment: angular#4000 (comment) While doing do I decided to take on this TODO: https://github.com/angular/protractor/blob/ccf02ab5f1070f0d7124318dc0099252f3c747e2/lib/browser.ts#L38 One possible issue here is that `ProtractorBrowser` only copies over methods, so it doesn't actually implement `WebDriver`'s interface. This isn't a problem right now, since `WebDriver`'s interface only has functions on it. But it could be a problem in the future.
I decided to address this comment:
#4000 (comment)
While doing do I decided to take on this TODO:
protractor/lib/browser.ts
Line 38 in ccf02ab
One possible issue here is that
ProtractorBrowser
only copies over methods, so it doesn't actuallyimplement
WebDriver
's interface. This isn't a problem right now, sinceWebDriver
's interfaceonly has functions on it. But it could be a problem in the future.
This is also weird in that
browser instanceof webdriver.WebDriver
is false but TypeScript probably thinks otherwise. An alternative option would be to do:Which would accomplish basically what the TODO suggested, but without ever actually inheriting from anyone