chore(types): make plugins.ts more strongly-typed #3685

thorn0 · 2016-10-29T23:35:46Z

Some code reformatting happened here because of changing ColumnLimit in .clang-format. I can extract it to a separate PR if needed. The main idea of this PR is to use the type definitions for the selenium-webdriver module and make the compiler infer better types for the members of the Plugins class.

cnishina

Some of these changes are clang related rather than making the plugins.ts more strongly-typed. This might be better as two separate PRs.

cnishina · 2016-10-30T01:07:26Z

lib/plugins.ts

@@ -1,7 +1,15 @@
-var webdriver = require('selenium-webdriver');
-import {Logger} from './logger';
 import * as Q from 'q';


initially I changed this from q to Q. This could be set to lowercase.

cnishina · 2016-10-30T01:14:35Z

lib/plugins.ts

 import {ConfigParser} from './configParser';
+import {Logger} from './logger';
+
+declare module 'q' {


If this is a missing typing in @types/q, then this should be filed against DefinitelyTyped types-2.0 branch. Also, a // TODO should be added to remove this definition when the DefinitelyTyped PR is merged.

It's a deprecated and undocumented method that Protractor still uses. It can be difficult to get the reviewers to accept such a PR as they usually want to see a link to API docs. So I'm not sure it'd be the right thing to do.

I'll look into whether we can just not use this method at all.

I did submit a PR to DefinitelyTyped.

cnishina · 2016-10-30T01:22:37Z

lib/plugins.ts

+   *
+   * @return The handler
+   */
+  private pluginFunFactory(funName: string, promiseType: PromiseType.Q, failReturnVal?: boolean):


promiseType: PromiseType. This could either be WebDriver or Q. I see some benefit defining several times to get the correct return type. If we define it this way:

private pluginFunFactory(funName: string, promsieType: PromiseType, failReturnVal?: boolean): (...args: any[]) => webdriver.promise.Promise<any[]> | (...args: any[]) => Q.Promise<any[]> { ...

does it have negative effects on the typings?

Hmmm... after thinking about this more. The way you had it was fine.

The most elegant solution would be not to use the PromiseType enum at all, and instead make the function generic and pass the promise implementation itself (q or webdriver.promise) instead of an enum value. It'd look like this:

private pluginFunFactory<T extend typeof q | typeof webdriver.promise>( funName: string, promiseImplementation: T, failReturnVal?: boolean) { ... }

Unfortunately, this doesn't work because of this: microsoft/TypeScript#11940

cnishina · 2016-10-30T01:32:08Z

@thorn0 thanks for another great PR. Just a few comments.

cnishina · 2016-10-31T21:14:48Z

Thanks! Just saw the separate PR #3686.

cnishina · 2016-11-02T19:04:36Z

Please rebase.

thorn0 · 2016-11-06T00:37:15Z

@cnishina please have a look

cnishina · 2016-11-07T19:27:28Z

lib/browser.ts

-      return this.driver.controlFlow().execute(() => {
-        return this.plugins_.onPageLoad();
-      });
+      return this.driver.controlFlow().execute(() => this.plugins_.onPageLoad()).then(() => {});


.then(() => {}) This appears to not do anything. Should this be removed?

That's a good question. First of all, it doesn't do nothing, it converts Promise<any[]> to Promise<void>, so that the return type of ProtractorBrowser#get is Promise<void>.

onPageLoad used to be typed simply as Function, but now that its type is inferred to be (...args: any[]) => webdriver.promise.Promise<any[]>, it became apparent that the returned promise is resolved with an array (the result of the .all(...) call in pluginFunFactory), and this array seems to be totally useless. Moreover, the presence of this array can also have unexpected effects in run time. E.g. we may pass a function to then expecting that its argument would be undefined whereas in fact it'd get an array.

That's why I'm thinking about moving .then(() => {}) from here to Plugins#pluginFunFactory, so that it generates functions of type (...args: any[]) => Promise<void>. What are your thoughts on this? Are probably among those functions (setup, onPrepare, etc.) such whose resolved array value isn't meaningless?

Okay, I found that the array is meaningful for waitForCondition... It's used in browser.ts:

return this.plugins_.waitForCondition().then((results: boolean[]) => { return results.reduce((x, y) => { return x && y; }, true); });

Which means .then(() => {}) can't be moved to pluginFunFactory.

None of the functions generated by pluginFunFactory are ever exposed to protractor users, so I'm not bothered by the fact that the promise resolves to something useless. Instead of this .then() block - which is really only here for type checking - why not just cast this to a () => void?

These functions are exposed indirectly. ProtractorBrowser#get is exposed. The very line we're discussing now is its return statement. So it's not just type checking, it's public API.

Good point 😄

cnishina · 2016-11-07T19:34:59Z

lib/plugins.ts

-      failReturnVal: any): any {
-    var deferred = promiseType == PromiseType.Q ? Q.defer() : webdriver.promise.defer();
-    var logError = (e: any) => {
+  private safeCallPluginFun(


@sjelin could you review the safeCallPluginFun changes?

cnishina · 2016-11-07T19:36:33Z

lib/element.ts

@@ -1061,7 +1061,7 @@ export class ElementFinder extends WebdriverWebElement {
 * @returns {ElementFinder} which identifies the located
 *     {@link webdriver.WebElement}
 */
-export let build$ = (element: ElementHelper, by: ProtractorBy) => {
+export let build$ = (element: ElementHelper, by: typeof By) => {


Let's revert this to ProtractorBy. ProtractorBy is composed with By functionality and currently does not inherit from By.

The thing is this function is actually called with WebDriver's By, not with Protractor's one. See browser.ts:

this.$ = build$(this.element, By); this.$$ = build$$(this.element, By);

Hence this change.

Yup. I agree with the change. Thanks for this fix.

cnishina · 2016-11-07T19:36:39Z

lib/element.ts

@@ -1092,7 +1092,7 @@ export let build$ = (element: ElementHelper, by: ProtractorBy) => {
 * @returns {ElementArrayFinder} which identifies the
 *     array of the located {@link webdriver.WebElement}s.
 */
-export let build$$ = (element: ElementHelper, by: ProtractorBy) => {
+export let build$$ = (element: ElementHelper, by: typeof By) => {


cnishina · 2016-11-07T19:37:07Z

lib/plugins.ts

-            'Plugin configuration did not contain a valid path or ' +
-            'inline definition.');
-      }
+        var pluginObj: ProtractorPlugin;


nit: use let over var

cnishina · 2016-11-07T19:38:46Z

lib/plugins.ts

-var webdriver = require('selenium-webdriver');
-import {Logger} from './logger';
-import * as Q from 'q';
+import * as q from 'q';


Thanks for fixing this. This is leftover from experimenting to get the 'Q' module to work with our typings.

However, in the docs for that module, the uppercase Q is used...

Yup, I think lowercase is fine.

cnishina · 2016-11-07T19:39:12Z

lib/plugins.ts

@@ -380,14 +377,11 @@ export class Plugins {
   * @return {Object} The results object
   */
  getResults() {
-    var results: {failedCount: number, specResults: any[]} = {failedCount: 0, specResults: []};
+    var results = {failedCount: 0, specResults: [] as SpecResult[]};


nit: Let's fix this too. Use let over var

cnishina · 2016-11-07T19:39:50Z

lib/plugins.ts

-    var result = this.pluginObjs.some((pluginObj: ProtractorPlugin) => {
-      return pluginObj.skipAngularStability;
-    });
+    var result = this.pluginObjs.some(pluginObj => pluginObj.skipAngularStability);


nit: Same nit fix for let over var

sjelin

A few minor questions but I like the change!

sjelin · 2016-11-07T20:46:11Z

lib/plugins.ts

-        path = ConfigParser.resolveFilePatterns(pluginConf.path, true, config.configDir)[0];
-        if (!path) {
-          throw new Error('Invalid path to plugin: ' + pluginConf.path);
+    if (config.plugins) {


As a rule, I don't like making changes which affect the compiled code when we're just trying to make the typing better. I'm alright with it in this case though since an if statement is probably better style than || [] anyway 😛

sjelin · 2016-11-07T20:46:32Z

lib/browser.ts

-      return this.driver.controlFlow().execute(() => {
-        return this.plugins_.onPageLoad();
-      });
+      return this.driver.controlFlow().execute(() => this.plugins_.onPageLoad()).then(() => {});


None of the functions generated by pluginFunFactory are ever exposed to protractor users, so I'm not bothered by the fact that the promise resolves to something useless. Instead of this .then() block - which is really only here for type checking - why not just cast this to a () => void?

sjelin · 2016-11-07T20:50:31Z

lib/plugins.ts

-      }
+        var pluginObj: ProtractorPlugin;
+        if (path) {
+          pluginObj = (<ProtractorPlugin>require(path));


Let's change this to pluginObj = require(path) as ProtractorPlugin; (I think the style guide changed after I wrote that line!)

sjelin · 2016-11-07T21:30:15Z

lib/plugins.ts

+  private safeCallPluginFun(
+      pluginObj: ProtractorPlugin, funName: string, args: any[], promiseType: PromiseType,
+      failReturnVal: any) {
+    let resultPromise: webdriver.promise.Promise<any>|q.Promise<any>;


Why this way instead of my way? Couldn't you have just given deferred the type webdriver.promise.Deferred | q.Deferred?

That's actually how it was initially done in the previous version of the current PR. But unfortunately these two deferreds aren't compatible. WebDriver's one has the fulfill method, whereas Q has resolve. It has fulfill as well in fact, but it's deprecated, undocumented, not included to the type definitions, and I don't think the PR that adds it is going to be merged.

Just in the beginning of plugins.ts, my initial version of this PR had something like

declare module 'q' { // definition for `fulfill` goes here }

but @cnishina has asked to remove this.

I myself tried to minimize changes that affect the compiled code. But on the other hand, stronger types sometimes reveal interesting facts that require such changes. E.g. the fact that a deprecated method of Q is used.

The 'q' issue should be fixed when your PR to DefinitelyTyped is merged. Is fulfill deprecated (https://github.com/kriskowal/q/blob/v1/q.js#L1141)? I believe that comment should be fixed in your DefinitelyTyped PR (DefinitelyTyped/DefinitelyTyped#12358).

Hm, probably you're right. I searched through the history of edits of the API reference. It turns out it never was documented. So we can't really call it deprecated.

I've updated that PR, but IMHO using undocumented methods isn't a much better thing than using deprecated ones.

sjelin · 2016-11-07T21:31:48Z

lib/plugins.ts

+    if (promiseType == PromiseType.Q) {
+      const deferred = q.defer<any>();
+      resultPromise = deferred.promise;
+      done = deferred.resolve;


Does this not have to be bound to the deferred object?

In both libraries, these methods are bound.

sjelin · 2016-11-07T21:31:56Z

lib/plugins.ts

+    } else {
+      const deferred = webdriver.promise.defer<any>();
+      resultPromise = deferred.promise;
+      done = deferred.fulfill;


Same as above

sjelin

👍 from me

sjelin · 2016-11-08T00:32:08Z

lib/browser.ts

-      return this.driver.controlFlow().execute(() => {
-        return this.plugins_.onPageLoad();
-      });
+      return this.driver.controlFlow().execute(() => this.plugins_.onPageLoad()).then(() => {});


Good point 😄

juliemr · 2016-11-09T22:06:47Z

lib/browser.ts

@@ -596,7 +594,7 @@ export class ProtractorBrowser extends Webdriver {
   *     the script and may be referenced using the `arguments` object.
   */
  addMockModule(name: string, script: string|Function, ...moduleArgs: any[]) {
-    this.mockModules_.push({name: name, script: script, args: moduleArgs});
+    this.mockModules_.push({name, script, args: moduleArgs});


I dislike the shorthand here, let's continue to use the more explicit form.

juliemr · 2016-11-09T22:15:15Z

lib/browser.ts

-  get(destination: string, opt_timeout?: number) {
-    let timeout = opt_timeout ? opt_timeout : this.getPageTimeout;
-
+  get(destination: string, opt_timeout = this.getPageTimeout) {


This should just be called timeout now.

juliemr · 2016-11-09T22:18:06Z

lib/plugins.ts

-          var specName = info.specName || (obj.name + ' Plugin Tests');
-          var assertion: any = {passed: passed};
+          const specName = info.specName || (obj.name + ' Plugin Tests');
+          const assertion: AssertionResult = {passed};


Here too with the object shortcut.

juliemr · 2016-11-09T22:23:07Z

LGTM with cleanups.

thorn0 · 2016-11-10T00:00:34Z

done

googlebot added the cla: yes label Oct 29, 2016

cnishina reviewed Oct 30, 2016

View reviewed changes

cnishina added the pr: needs cleanup label Oct 30, 2016

cnishina suggested changes Nov 7, 2016

View reviewed changes

sjelin suggested changes Nov 7, 2016

View reviewed changes

sjelin approved these changes Nov 8, 2016

View reviewed changes

heathkit added this to the 4.0.11 milestone Nov 9, 2016

cnishina approved these changes Nov 9, 2016

View reviewed changes

juliemr reviewed Nov 9, 2016

View reviewed changes

chore(types): make plugins.ts more strongly-typed

d1cfc97

cnishina merged commit e9061b3 into angular:master Nov 10, 2016

chore(types): make plugins.ts more strongly-typed #3685

chore(types): make plugins.ts more strongly-typed #3685

Conversation

thorn0 commented Oct 29, 2016

cnishina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnishina Oct 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnishina commented Oct 30, 2016

cnishina commented Oct 31, 2016

cnishina commented Nov 2, 2016

thorn0 commented Nov 6, 2016

Choose a reason for hiding this comment

thorn0 Nov 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjelin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thorn0 Nov 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjelin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliemr commented Nov 9, 2016

thorn0 commented Nov 10, 2016

cnishina Oct 30, 2016 •

edited

Loading

thorn0 Nov 7, 2016 •

edited

Loading

thorn0 Nov 7, 2016 •

edited

Loading