Add debug method to the public API #2677

TsvetanMilanov · 2017-03-30T18:15:12Z

We need to add debug service to the public API of the {N} CLI. This service should have method which starts debugging and returns the chrome devtools url. With this service it will be possible to use the CLI as a library and start debugging.
This PR includes:

Return the results from the action in devicesService.execute(action)
Remove the this.$options from both android and ios debug services
Remove the projectData dependency from both android and ios debug services
Add the new debug service which will be used in the public API

Common lib reference: telerik/mobile-cli-lib#927

Mitko-Kerezov · 2017-03-31T12:43:30Z

lib/commands/debug.ts

 		protected $platformService: IPlatformService,
 		protected $projectData: IProjectData,
 		protected $options: IOptions,
 		protected $platformsData: IPlatformsData) {
-			this.$projectData.initializeProjectData();
-		}
+		this.$projectData.initializeProjectData(this.$options.path);


You can skip passing this.$options.path here - it'll default to it anyways

Mitko-Kerezov · 2017-03-31T13:39:34Z

lib/services/debug-data-service.ts

+	}
+
+	private getPathToAppPackage(debugService: IPlatformDebugService, options: IOptions, buildConfig: IBuildConfig): string {
+		if (debugService.platform === this.$devicePlatformsConstants.Android) {


I think it'd be better to use $mobileHelper.isAndroidPlatform here in case the casing differs

Mitko-Kerezov · 2017-03-31T13:43:05Z

lib/services/debug-data-service.ts

+			} else {
+				return null;
+			}
+		} else if (debugService.platform === this.$devicePlatformsConstants.iOS) {


Same here $mobileHelper.isiOSPlatform

Mitko-Kerezov · 2017-03-31T13:44:15Z

lib/services/debug-service-base.ts

+import { EventEmitter } from "events";
+
+export abstract class DebugServiceBase extends EventEmitter implements IPlatformDebugService {
+	constructor() {


Isn't such a constructor gratuitous?

Mitko-Kerezov · 2017-03-31T13:47:49Z

lib/services/debug-service.ts

+		// TODO: Check if app is running.
+		const isAppRunning = true;
+		let result: string[];
+		if (device.deviceInfo.platform === this.$devicePlatformsConstants.iOS) {


Same remark about $mobileHelper.isiOSPlatform

Mitko-Kerezov · 2017-03-31T13:48:27Z

lib/services/debug-service.ts

+			}
+
+			result = await debugService.debug(debugData, debugOptions);
+		} else if (device.deviceInfo.platform === this.$devicePlatformsConstants.Android) {


$mobileHelper

Mitko-Kerezov · 2017-03-31T13:53:19Z

lib/services/ios-debug-service.ts

-		this.$devicesService.execute(async (device: Mobile.IiOSDevice) => await device.isEmulator ? this.emulatorDebugBrk(projectData, buildConfig) : this.debugBrkCore(device, projectData));
+	public async debugStart(debugData: IDebugData, debugOptions: IDebugOptions): Promise<void> {
+		await this.$devicesService.initialize({ platform: this.platform, deviceId: debugData.deviceIdentifier });
+		const action = async (device: Mobile.IiOSDevice) => await device.isEmulator ? this.emulatorDebugBrk(debugData, false, debugOptions) : this.debugBrkCore(device, debugData, debugOptions);


The await operator here seems misplaced

Mitko-Kerezov · 2017-03-31T13:58:52Z

lib/services/test-execution-service.ts

 				if (this.$options.debugBrk) {
-					await this.getDebugService(platform).debug(projectData, buildConfig);
+					const debugService = this.getDebugService(platform); const deployOptions: IDeployPlatformOptions = {


deployOptions can be declared above the if statement in order to avoid repetition

That's why they've been declared there so far 😃

Mitko-Kerezov · 2017-03-31T13:59:30Z

test/debug.ts

@@ -43,6 +43,9 @@ function createTestInjector(): IInjector {
 	testInjector.register("projectTemplatesService", {});
 	testInjector.register("xmlValidator", {});
 	testInjector.register("npm", {});
+	testInjector.register("debugDataService", {
+		createDebugData: () => { return {}; }


You can use the short-hand syntax here:
createDebugData: () => ({})

rosen-vladimirov · 2017-04-02T21:58:57Z

lib/services/android-debug-service.ts

-			}
-
-			port = candidatePort;
+			port = await getAvailablePort(40000);


why do we start from 40000?

Because we want to start chrome devtools on port 40000 each time we start debugging. That's why we don't get random free port. If we get random free port, the users should copy the url each time tns debug is executed.

rosen-vladimirov · 2017-04-02T22:18:39Z

lib/services/debug-data-service.ts

+				const platformData = this.getPlatformData(debugService);
+
+				return this.$platformService.getLatestApplicationPackageForDevice(platformData, buildConfig).packageName;
+			} else {


do we really need the else? Can't you place the return null at the end of the method?

rosen-vladimirov · 2017-04-02T22:19:48Z

lib/services/debug-data-service.ts

+		}
+	}
+
+	protected getPlatformData(debugService: IPlatformDebugService): IPlatformData {


why is this method protected and not private?

rosen-vladimirov · 2017-04-02T22:21:39Z

lib/services/debug-service.ts

+import { EventEmitter } from "events";
+import { CONNECTION_ERROR_EVENT_NAME } from "../constants";
+
+class DebugService extends EventEmitter {


shouldn't you implement IDebugService as well ?

This debug service has different debug method. In ios/android debug service the debug method returns array of the chrome devtools urls. In this service we want to start debugging only on one device and return the url only for it.

rosen-vladimirov · 2017-04-02T22:24:27Z

lib/services/debug-service.ts

+		const device = this.$devicesService.getDeviceByIdentifier(debugData.deviceIdentifier);
+		const debugService = this.getDebugService(device);
+
+		debugService.on(CONNECTION_ERROR_EVENT_NAME, (e: Error) => this.emit(CONNECTION_ERROR_EVENT_NAME, e));


who will remove this handler? In case you call debug method three times, you'll have three handlers for the error event. So in case an error is raised, the one who listens for DebugService's CONNECTION_ERROR_EVENT_NAME event will receive three errors.

Also why the handler is attached before the if (!device) statement. It looks like the handler has meaning only when we have a device.

rosen-vladimirov · 2017-04-02T22:30:55Z

lib/services/debug-service.ts

+
+			result = await debugService.debug(debugData, debugOptions);
+		} else if (device.deviceInfo.platform === this.$devicePlatformsConstants.Android) {
+			debugOptions.client = true;


shouldn't this be valid for iOS instead of Android?

debugOptions.client in the android debug service is the same as debugOptions.chrome in ios debug service. I changed the android debug service to use debugOptions.chrome.

rosen-vladimirov · 2017-04-02T22:32:58Z

lib/services/ios-debug-service.ts

-	private async deviceDebugBrk(projectData: IProjectData, buildConfig: IBuildConfig, shouldBreak?: boolean): Promise<void> {
-		await this.$devicesService.initialize({ platform: this.platform, deviceId: this.$options.device });
-		this.$devicesService.execute(async (device: iOSDevice.IOSDevice) => {
+	private async deviceDebugBrk(debugData: IDebugData, shouldBreak: boolean, debugOptions: IDebugOptions): Promise<string[]> {


isn't shouldBreak a debug option? Can't we place it in debugOptions object?
P.S. I do not like passing boolean arguments.

rosen-vladimirov · 2017-04-02T22:33:51Z

lib/services/ios-debug-service.ts

+			return result;
+		};
+
+		const result = await this.$devicesService.execute(action, this.getCanExecuteAction(debugData.deviceIdentifier));


results sounds better

rosen-vladimirov · 2017-04-02T22:34:27Z

lib/services/ios-debug-service.ts

+	private async deviceStart(debugData: IDebugData, debugOptions: IDebugOptions): Promise<string[]> {
+		await this.$devicesService.initialize({ platform: this.platform, deviceId: debugData.deviceIdentifier });
+		const action = async (device: Mobile.IiOSDevice) => device.isEmulator ? await this.emulatorStart(debugData, debugOptions) : await this.deviceStartCore(device, debugData, debugOptions);
+		const result = await this.$devicesService.execute(action, this.getCanExecuteAction(debugData.deviceIdentifier));


rosen-vladimirov · 2017-04-02T22:36:25Z

lib/services/test-execution-service.ts

 				if (this.$options.debugBrk) {
-					await this.getDebugService(platform).debug(projectData, buildConfig);
+					const debugService = this.getDebugService(platform); const deployOptions: IDeployPlatformOptions = {


That's why they've been declared there so far 😃

Mitko-Kerezov

LGTM

TsvetanMilanov self-assigned this Mar 30, 2017

TsvetanMilanov requested a review from rosen-vladimirov March 30, 2017 18:15

TsvetanMilanov force-pushed the milanov/add-debug-method-to-the-public-api branch 3 times, most recently from e9548d6 to 6ca7280 Compare March 31, 2017 11:29

Mitko-Kerezov suggested changes Mar 31, 2017

View reviewed changes

rosen-vladimirov suggested changes Apr 2, 2017

View reviewed changes

TsvetanMilanov force-pushed the milanov/add-debug-method-to-the-public-api branch from 61bf525 to 6bd76ad Compare April 5, 2017 15:19

Mitko-Kerezov approved these changes Apr 6, 2017

View reviewed changes

TsvetanMilanov force-pushed the milanov/add-debug-method-to-the-public-api branch from 6bd76ad to 1cd6874 Compare April 7, 2017 10:26

Add debug method to the public API

b79f33e

TsvetanMilanov force-pushed the milanov/add-debug-method-to-the-public-api branch from 1cd6874 to 3d9c6b1 Compare April 7, 2017 10:30

Fix PR comments

8f79755

TsvetanMilanov force-pushed the milanov/add-debug-method-to-the-public-api branch from 3d9c6b1 to 8f79755 Compare April 7, 2017 10:37

rosen-vladimirov approved these changes Apr 7, 2017

View reviewed changes

TsvetanMilanov merged commit 1b19adf into master Apr 7, 2017

TsvetanMilanov deleted the milanov/add-debug-method-to-the-public-api branch April 7, 2017 12:19

Uh oh!

Add debug method to the public API #2677

Add debug method to the public API #2677

Uh oh!

Conversation

TsvetanMilanov commented Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mitko-Kerezov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TsvetanMilanov commented Mar 30, 2017 •

edited

Loading