Remove ios native dependencies by TsvetanMilanov · Pull Request #889 · telerik/mobile-cli-lib

TsvetanMilanov · 2017-02-06T10:12:34Z

We need to remove our native dependencies - ref and ffi because they are casuing problems with each version of NodeJS which has new version of V8 in it. We are using ref and ffi to communicate with iTunes and execute fs and application management operations on iOS devices. This operations are moved in new dependency - device-lib. The device lib has JavaScript wrapper which we use in the CLI. Everything in the ios device file system and ios device application manager is replaced with the exposed methods from the device lib.

Mitko-Kerezov

Please fix relevant comments

Mitko-Kerezov · 2017-02-08T09:10:39Z

mobile/ios/device/ios-application-manager.ts

-				]
-			*/
+		const mappedApps = _.map(applicationsOnDevice, app => {
+			return {


You can use the short syntax here:

_.map(applicationsOnDevice, app => ({ applicationIdentifier: app.CFBundleIdentifier, isLiveSyncSupported: app.IceniumLiveSyncEnabled, configuration: app.configuration, deviceIdentifier: this.device.deviceInfo.identifier }));

Mitko-Kerezov · 2017-02-08T09:15:32Z

mobile/ios/device/ios-application-manager.ts

-				}));
-				this.applicationsLiveSyncInfos = this.applicationsLiveSyncInfos.concat(currentList);
-			});
+		this.applicationsLiveSyncInfos = this.applicationsLiveSyncInfos.concat(mappedApps);


Since you're setting this.applicationsLiveSyncInfos = [] 10 rows above, IMO there's no need to concat here.
You can skip this.applicationsLiveSyncInfos = [] from above and just reassign here:
this.applicationsLiveSyncInfos = mappedApps;

Mitko-Kerezov · 2017-02-08T09:16:00Z

mobile/ios/device/ios-application-manager.ts

 	public async startApplication(appIdentifier: string): Promise<void> {
 		if (this.$hostInfo.isWindows && !this.$staticConfig.enableDeviceRunCommandOnWindows) {
-			this.$errors.fail("$%s device run command is not supported on Windows for iOS devices.", this.$staticConfig.CLIENT_NAME.toLowerCase());
+			// this.$errors.fail("$%s device run command is not supported on Windows for iOS devices.", this.$staticConfig.CLIENT_NAME.toLowerCase());


Do we need this?

I believe we need it.

Mitko-Kerezov · 2017-02-08T09:26:26Z

mobile/ios/device/ios-device-file-system.ts

+		try {
+			await settlePromises(await this.$iosDeviceOperations.uploadFiles(filesToUpload));
+		} catch (err) {
+			if (err.deviceId === this.device.deviceInfo.identifier) {


settlePromises alters the thrown error, so you won't get an object that has a deviceId in the catch clause

Mitko-Kerezov · 2017-02-08T09:44:00Z

mobile/ios/device/ios-device-operations.ts

+		}
+	}
+
+	@cache()


I don't think we should @cache this - what if we want to start a second device log (e.g. we have two devices attached)

Mitko-Kerezov · 2017-02-08T09:58:24Z

mobile/ios/device/ios-device.ts

+import * as net from "net";

 export class IOSDevice implements Mobile.IiOSDevice {
 	// iOS errors are described here with HEX representation


IMO you can delete those comments too - they're no longer relevant

Mitko-Kerezov · 2017-02-08T10:13:57Z

mobile/ios/device/ios-device.ts

+
+	public async openDeviceLogStream(): Promise<void> {
+		if (this.deviceInfo.status !== constants.UNREACHABLE_STATUS) {
+			await this.$iosDeviceOperations.startDeviceLog(this.deviceInfo.identifier, (data: string) => {


There's no need to await this - it doesn't return a Promise

rosen-vladimirov

The title says: Remove ios native dependencies
However the PR just adds dependency in package.json. Shouldn't we actually delete the native dependencies from package.json?

rosen-vladimirov · 2017-02-08T23:13:53Z

mobile/ios/device/ios-application-manager.ts

-			this.$logger.trace(`Error while uninstalling application ${e}.`);
-		}
+		await this.$iosDeviceOperations.uninstall(appIdentifier, [this.device.deviceInfo.identifier], (err: IDeviceOperationError) => {
+			this.$logger.warn(`Failed to uninstall install ${appIdentifier} on device with identifier ${err.deviceIdentifier}`);


which operation failed - unisntall or install ?

rosen-vladimirov · 2017-02-08T23:28:52Z

commands/device/get-file.ts


 	public async execute(args: string[]): Promise<void> {
 		await this.$devicesService.initialize({ deviceId: this.$options.device, skipInferPlatform: true });
+		let appIdentifier = args[1];


as we modify the command, shouldn't we delete the $options.app that has been used before (not in the command itself, but in the service behind it. If I understand correctly, we'll support getting of file only from application sandbox, so the $options.app is a mandatory argument now. So... let's remove $options.app

rosen-vladimirov · 2017-02-08T23:30:19Z

mobile/ios/device/ios-application-manager.ts

 	public async startApplication(appIdentifier: string): Promise<void> {
 		if (this.$hostInfo.isWindows && !this.$staticConfig.enableDeviceRunCommandOnWindows) {
-			this.$errors.fail("$%s device run command is not supported on Windows for iOS devices.", this.$staticConfig.CLIENT_NAME.toLowerCase());
+			// this.$errors.fail("$%s device run command is not supported on Windows for iOS devices.", this.$staticConfig.CLIENT_NAME.toLowerCase());


I believe we need it.

Plamen5kov · 2017-02-28T09:47:15Z

related issue: NativeScript/nativescript-cli#2562

We need to remove our native dependencies - ref and ffi because they are casuing problems with each version of NodeJS which has new version of V8 in it. We are using ref and ffi to communicate with iTunes and execute fs and application management operations on iOS devices. This operations are moved in new dependency - device-lib. The device lib has JavaScript wrapper which we use in the CLI. Everything in the ios device file system and ios device application manager is replaced with the exposed methods from the device lib.

Since we use the ios-device-lib to connect to the device backend port for debugging, the socket returned from the device can be used only in the ios-device lib. That's why we need to change the logic for creating the Node.js socket in the CLI. The ios-device-lib will expose the device socket on a specific port in the localhost. After we connect to the exposed socket we can continue to work like we used to.

justcodebuilduser · 2017-03-10T17:42:34Z

💔

TsvetanMilanov · 2017-03-14T10:07:39Z

run ci

justcodebuilduser · 2017-03-14T10:09:21Z

❤️

justcodebuilduser · 2017-03-14T10:14:42Z

❤️

TsvetanMilanov added the enhancement label Feb 6, 2017

TsvetanMilanov added this to the AB 3.7.0 milestone Feb 6, 2017

TsvetanMilanov self-assigned this Feb 6, 2017

TsvetanMilanov requested review from Mitko-Kerezov and rosen-vladimirov February 6, 2017 10:12

TsvetanMilanov force-pushed the milanov/remove-ios-native-dependencies branch 2 times, most recently from a0904aa to 97cbce9 Compare February 6, 2017 16:01

Mitko-Kerezov suggested changes Feb 8, 2017

View reviewed changes

rosen-vladimirov reviewed Feb 8, 2017

View reviewed changes

TsvetanMilanov force-pushed the milanov/remove-ios-native-dependencies branch 2 times, most recently from c752b38 to 99e1148 Compare February 9, 2017 12:13

This was referenced Feb 14, 2017

Remove ios native dependencies NativeScript/nativescript-cli#2495

Merged

Remove ios native dependencies Icenium/icenium-cli#1636

Merged

TsvetanMilanov force-pushed the milanov/remove-ios-native-dependencies branch 2 times, most recently from cda297e to f06b56e Compare February 23, 2017 14:34

rosen-vladimirov mentioned this pull request Feb 28, 2017

Fix QStartNoAckMode #903

Closed

Plamen5kov mentioned this pull request Feb 28, 2017

Changes in App_Resources during prepare triggers a build NativeScript/nativescript-cli#2562

Closed

TsvetanMilanov force-pushed the milanov/remove-ios-native-dependencies branch from f06b56e to eaec133 Compare February 28, 2017 12:43

Plamen5kov mentioned this pull request Feb 28, 2017

added forgotten await #902

Closed

TsvetanMilanov force-pushed the milanov/remove-ios-native-dependencies branch from 8ebc7b9 to a82b75c Compare March 2, 2017 12:20

TsvetanMilanov added 5 commits March 3, 2017 14:05

Fix PR comments

e57f215

Update ios-device-lib bersion

d0498b5

Change the notification logic

e92b8c5

Override net.Socket write method

741a91d

Mitko-Kerezov force-pushed the milanov/remove-ios-native-dependencies branch from f978b57 to 741a91d Compare March 3, 2017 12:05

Add receive message from ios device socket logic

6fd98a6

TsvetanMilanov force-pushed the milanov/remove-ios-native-dependencies branch from f2d2993 to 220a852 Compare March 10, 2017 09:15

TsvetanMilanov force-pushed the milanov/remove-ios-native-dependencies branch from 220a852 to cfd19f2 Compare March 10, 2017 17:40

rosen-vladimirov approved these changes Mar 14, 2017

View reviewed changes

Update ios-device-lib-version

32ecf38

TsvetanMilanov force-pushed the milanov/remove-ios-native-dependencies branch from 802b22c to 32ecf38 Compare March 14, 2017 10:12

Mitko-Kerezov approved these changes Mar 14, 2017

View reviewed changes

TsvetanMilanov merged commit 6bcc88c into master Mar 14, 2017

TsvetanMilanov deleted the milanov/remove-ios-native-dependencies branch March 14, 2017 11:57

Conversation

TsvetanMilanov commented Feb 6, 2017

Uh oh!

Mitko-Kerezov left a comment

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!

rosen-vladimirov left a comment

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!

Plamen5kov commented Feb 28, 2017

Uh oh!

justcodebuilduser commented Mar 10, 2017

Uh oh!

TsvetanMilanov commented Mar 14, 2017

Uh oh!

justcodebuilduser commented Mar 14, 2017

Uh oh!

justcodebuilduser commented Mar 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants