Skip to content
This repository was archived by the owner on Jul 29, 2024. It is now read-only.

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

Merged
merged 1 commit into from
Nov 10, 2016
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 26 additions & 44 deletions lib/browser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {ActionSequence, Capabilities, Command as WdCommand, FileDetector, Options, promise as wdpromise, Session, TargetLocator, TouchSequence, until, WebDriver, WebElement} from 'selenium-webdriver';
import {ActionSequence, By, Capabilities, Command as WdCommand, FileDetector, ICommandName, Options, promise as wdpromise, Session, TargetLocator, TouchSequence, until, WebDriver, WebElement} from 'selenium-webdriver';
import * as url from 'url';

import {DebugHelper} from './debugger';
Expand All @@ -9,10 +9,11 @@ import {Locator, ProtractorBy} from './locators';
import {Logger} from './logger';
import {Plugins} from './plugins';

let clientSideScripts = require('./clientsidescripts');
let webdriver = require('selenium-webdriver');
let Command = require('selenium-webdriver/lib/command').Command;
let CommandName = require('selenium-webdriver/lib/command').Name;
const clientSideScripts = require('./clientsidescripts');
const webdriver = require('selenium-webdriver');
// TODO: fix the typings for selenium-webdriver/lib/command
const Command = require('selenium-webdriver/lib/command').Command as typeof WdCommand;
const CommandName = require('selenium-webdriver/lib/command').Name as ICommandName;

// jshint browser: true

Expand Down Expand Up @@ -244,7 +245,7 @@ export class ProtractorBrowser extends Webdriver {
* @type {Array<{name: string, script: function|string, args:
* Array.<string>}>}
*/
mockModules_: any[];
mockModules_: {name: string, script: string|Function, args: any[]}[];

/**
* If specified, start a debugger server at specified port instead of repl
Expand Down Expand Up @@ -279,8 +280,8 @@ export class ProtractorBrowser extends Webdriver {
let methodsToSync = ['getCurrentUrl', 'getPageSource', 'getTitle'];

// Mix all other driver functionality into Protractor.
Object.getOwnPropertyNames(webdriver.WebDriver.prototype).forEach((method: string) => {
if (!this[method] && typeof(webdriverInstance as any)[method] == 'function') {
Object.getOwnPropertyNames(WebDriver.prototype).forEach(method => {
if (!this[method] && typeof(webdriverInstance as any)[method] === 'function') {
if (methodsToSync.indexOf(method) !== -1) {
ptorMixin(this, webdriverInstance, method, this.waitForAngular.bind(this));
} else {
Expand All @@ -291,8 +292,8 @@ export class ProtractorBrowser extends Webdriver {

this.driver = webdriverInstance;
this.element = buildElementHelper(this);
this.$ = build$(this.element, webdriver.By);
this.$$ = build$$(this.element, webdriver.By);
this.$ = build$(this.element, By);
this.$$ = build$$(this.element, By);
this.baseUrl = opt_baseUrl || '';
this.rootEl = opt_rootElement || 'body';
this.ignoreSynchronization = false;
Expand Down Expand Up @@ -442,7 +443,7 @@ export class ProtractorBrowser extends Webdriver {

let runWaitForAngularScript: () => wdpromise.Promise<any> = () => {
if (this.plugins_.skipAngularStability()) {
return webdriver.promise.fulfilled();
return wdpromise.fulfilled();
} else if (this.rootEl) {
return this.executeAsyncScript_(
clientSideScripts.waitForAngular, 'Protractor.waitForAngular()' + description,
Expand Down Expand Up @@ -472,9 +473,7 @@ export class ProtractorBrowser extends Webdriver {
.then(() => {
return this.driver.wait(() => {
return this.plugins_.waitForCondition().then((results: boolean[]) => {
return results.reduce((x, y) => {
return x && y;
}, true);
return results.reduce((x, y) => x && y, true);
});
}, this.allScriptsTimeout, 'Plugins.waitForCondition()');
});
Expand Down Expand Up @@ -580,8 +579,7 @@ export class ProtractorBrowser extends Webdriver {
* Add a module to load before Angular whenever Protractor.get is called.
* Modules will be registered after existing modules already on the page,
* so any module registered here will override preexisting modules with the
* same
* name.
* same name.
*
* @example
* browser.addMockModule('modName', function() {
Expand Down Expand Up @@ -629,9 +627,7 @@ export class ProtractorBrowser extends Webdriver {
* @returns {Array.<!string|Function>} The list of mock modules.
*/
getRegisteredMockModules(): Array<string|Function> {
return this.mockModules_.map((module) => {
return module.script;
});
return this.mockModules_.map(module => module.script);
};

/**
Expand Down Expand Up @@ -661,9 +657,7 @@ export class ProtractorBrowser extends Webdriver {
* @param {number=} opt_timeout Number of milliseconds to wait for Angular to
* start.
*/
get(destination: string, opt_timeout?: number) {
let timeout = opt_timeout ? opt_timeout : this.getPageTimeout;

get(destination: string, timeout = this.getPageTimeout) {
destination = this.baseUrl.indexOf('file://') === 0 ? this.baseUrl + destination :
url.resolve(this.baseUrl, destination);
let msg = (str: string) => {
Expand All @@ -672,17 +666,14 @@ export class ProtractorBrowser extends Webdriver {

if (this.ignoreSynchronization) {
this.driver.get(destination);
return this.driver.controlFlow().execute(() => {
return this.plugins_.onPageLoad();
});
return this.driver.controlFlow().execute(() => this.plugins_.onPageLoad()).then(() => {});
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@thorn0 thorn0 Nov 7, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point 😄

}

let deferred = webdriver.promise.defer();
let deferred = wdpromise.defer<void>();

this.driver.get(this.resetUrl).then(null, deferred.reject);
this.executeScriptWithDescription(
'window.name = "' + DEFER_LABEL + '" + window.name;' +

'window.location.replace("' + destination + '");',
msg('reset url'))
.then(null, deferred.reject);
Expand Down Expand Up @@ -741,16 +732,11 @@ export class ProtractorBrowser extends Webdriver {
let self = this;
function loadMocks(angularVersion: number) {
if (angularVersion === 1) {
// At this point, Angular will pause for us until
// angular.resumeBootstrap
// is called.
// At this point, Angular will pause for us until angular.resumeBootstrap is called.
let moduleNames: string[] = [];
for (let i = 0; i < self.mockModules_.length; ++i) {
let mockModule = self.mockModules_[i];
let name = mockModule.name;
for (const {name, script, args} of self.mockModules_) {
moduleNames.push(name);
let executeScriptArgs =
[mockModule.script, msg('add mock module ' + name)].concat(mockModule.args);
let executeScriptArgs = [script, msg('add mock module ' + name), ...args];
self.executeScriptWithDescription.apply(self, executeScriptArgs)
.then(
null,
Expand All @@ -776,13 +762,9 @@ export class ProtractorBrowser extends Webdriver {
}

this.driver.controlFlow().execute(() => {
return self.plugins_.onPageStable().then(
() => {
deferred.fulfill();
},
(error: Error) => {
deferred.reject(error);
});
return this.plugins_.onPageStable().then(() => {
deferred.fulfill();
}, deferred.reject);
});

return deferred.promise;
Expand Down Expand Up @@ -885,7 +867,7 @@ export class ProtractorBrowser extends Webdriver {
debugger() {
// jshint debug: true
this.driver.executeScript(clientSideScripts.installInBrowser);
webdriver.promise.controlFlow().execute(() => {
wdpromise.controlFlow().execute(() => {
debugger;
}, 'add breakpoint to control flow');
}
Expand Down Expand Up @@ -944,7 +926,7 @@ export class ProtractorBrowser extends Webdriver {
pause(opt_debugPort?: number): webdriver.promise.Promise<any> {
if (this.debugHelper.isAttached()) {
logger.info('Encountered browser.pause(), but debugger already attached.');
return webdriver.promise.fulfilled(true);
return wdpromise.fulfilled(true);
}
let debuggerClientPath = __dirname + '/debugger/clients/wddebugger.js';
let onStartFn = (firstTime: boolean) => {
Expand Down
4 changes: 3 additions & 1 deletion lib/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import {PluginConfig} from './plugins';

export interface Config {
// ---------------------------------------------------------------------------
// ----- How to connect to Browser Drivers -----------------------------------
Expand Down Expand Up @@ -563,7 +565,7 @@ export interface Config {
/**
* See docs/plugins.md
*/
plugins?: Array<any>;
plugins?: PluginConfig[];

/**
* Turns off source map support. Stops protractor from registering global
Expand Down
6 changes: 3 additions & 3 deletions lib/element.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import {By, error, ILocation, ISize, promise as wdpromise, WebDriver, WebElement
import {ElementHelper} from './browser';
import {ProtractorBrowser} from './browser';
import {IError} from './exitCodes';
import {Locator, ProtractorBy} from './locators';
import {Locator} from './locators';
import {Logger} from './logger';

let webdriver = require('selenium-webdriver');
Expand Down Expand Up @@ -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) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

return (selector: string) => {
return element(by.css(selector));
};
Expand Down Expand Up @@ -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) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

return (selector: string) => {
return element.all(by.css(selector));
};
Expand Down
Loading