chore(types): fix transpile errors related to IThenable #3650
Conversation
cnishina
force-pushed
the
transpile_issues
branch
from
b121d58 to
b111c02
Compare
cnishina
changed the title
| function loadMocks(angularVersion: number) { | ||
| if (angularVersion === 1) { | ||
| function loadMocks(angularVersion: string) { | ||
| if (angularVersion == '1') { |
There was a problem hiding this comment.
testForAngular returns ver as number. This line is where string needs to be replaced with number.
There was a problem hiding this comment.
This change is to satisfy TypeScript, loadMocks must have a string value. Also, since angularVersion is a string, it must then be compared to a string value. After the files are transpiled, the value '1' and 1 should not matter.
There was a problem hiding this comment.
But why must loadMocks have a string value? Because it's inferred from the type of the angularTestResult variable whose type is wrong.
There was a problem hiding this comment.
Good catch. Thanks! Fixed.
| @@ -61,7 +64,7 @@ export class DriverProvider { | |||
| if (driver.getSession() === undefined) { | |||
| deferred.resolve(); | |||
| } else { | |||
| driver.getSession().then((session_: string) => { | |||
| driver.getSession().then((session_: Session) => { | |||
There was a problem hiding this comment.
Why not just delete this type annotation? The compiler is able to infer the type of session_.
| }); | ||
| // Resolve the list of Promise<List<child_web_elements>> and merge | ||
| // into a single list | ||
| return wdpromise.all(childrenPromiseList).then((resolved: any) => { |
There was a problem hiding this comment.
Why any? resolved seems to be WebElement[][]. But again I think it's better just to delete the type annotations here. Excessive type annotations are a kind of type assertions, they prevent the compiler from doing its job of finding type errors.
| return wdpromise.all(childrenPromiseList).then((resolved: any) => { | ||
| return resolved.reduce( | ||
| (childrenList: WebElement[], | ||
| resolvedE: webdriver.WebElement) => { |
There was a problem hiding this comment.
These annotations seem to be wrong and excessive too.
There was a problem hiding this comment.
Sure. Removed the types.
| return fn(actionResults[0]); | ||
| }, errorFn); | ||
| }; | ||
| this.then = |
There was a problem hiding this comment.
I think this function should be made generic as well as the other then functions above.
There was a problem hiding this comment.
I don't think this is a good case for generics, because we don't actually know what value it will return based on the arguments to the function - it's based on what has been called in the chain.
|
Thanks @thorn0 for the review! 🎉 |
| return fn(actionResults[0]); | ||
| }, errorFn); | ||
| }; | ||
| this.then = |
There was a problem hiding this comment.
I don't think this is a good case for generics, because we don't actually know what value it will return based on the arguments to the function - it's based on what has been called in the chain.
No description provided.