Kddimitrov/android livesync sockets #3746

KristianDD · 2018-07-12T16:46:22Z

PR Checklist

The PR title follows our guidelines: https://github.com/NativeScript/NativeScript/blob/master/CONTRIBUTING.md#commit-messages.
There is an issue for the bug/feature this PR is for. To avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it.
You have signed the CLA.
All existing tests are passing: https://github.com/NativeScript/nativescript-cli/blob/master/CONTRIBUTING.md#contribute-to-the-code-base
Tests for the changes are included. - will be written during review
Add README.md for Public API of androidLivesyncLibrary

What is the current behavior?

Currently all files are transferred to the file system of the deice

What is the new behavior?

All files are transferred directly though the socket

Will log issue in CLI
Runtime issue: NativeScript/android#932

dtopuzov · 2018-07-13T06:59:23Z

test

Fatme · 2018-07-16T09:44:04Z

lib/definitions/livesync.d.ts

+	 * @param configuration - The configuration to the socket connection.
+	 * @returns {Promise<void>}
+	 */
+	connect(configuration: ILivesyncToolConfiguration): Promise<void>;


Should be IAndroidLivesyncToolConfiguration

Fatme · 2018-07-16T10:03:47Z

lib/services/livesync/android-device-livesync-sockets-service.ts

+	}
+
+	public async removeFiles(deviceAppData: Mobile.IDeviceAppData, localToDevicePaths: Mobile.ILocalToDevicePathData[], projectFilesPath: string): Promise<void> {
+		await this.livesyncTool.removeFiles(_.map(localToDevicePaths, (element: any) => { return element.filePath; }));


return this.livesyncTool.removeFiles(_.map(localToDevicePaths, (element: any) => element.filePath; ));

or even the _.map(users, 'filePath') shorthand as we are already using any here.

Fatme · 2018-07-16T10:05:07Z

lib/services/livesync/android-device-livesync-sockets-service.ts

+		return transferredFiles;
+	}
+
+	private async _transferFiles(deviceAppData: Mobile.IDeviceAppData, localToDevicePaths: Mobile.ILocalToDevicePathData[], projectFilesPath: string): Promise<Mobile.ILocalToDevicePathData[]> {


deviceAppData: Mobile.IDeviceAppData parameter is not used inside the function, so we can remove it.

The same for projectFilesPath parameter

Fatme · 2018-07-16T10:08:34Z

lib/services/livesync/android-device-livesync-sockets-service.ts

+	private async _transferFiles(deviceAppData: Mobile.IDeviceAppData, localToDevicePaths: Mobile.ILocalToDevicePathData[], projectFilesPath: string): Promise<Mobile.ILocalToDevicePathData[]> {
+		await this.livesyncTool.sendFiles(localToDevicePaths.map(localToDevicePathData => localToDevicePathData.getLocalPath()));
+
+		return localToDevicePaths;


Do we need here to return the all passed file paths or we need to return all transferred files?

DimitarTachev

I believe we can also cover most of the story with unit tests.

DimitarTachev · 2018-07-16T14:05:21Z

lib/services/livesync/android-livesync-library.ts

+				if (this.isOperationInProgress(id)) {
+					this.handleSocketError(socketId, "Sync operation is taking too long");
+				}
+			}, 60000);


60000 -> const

DimitarTachev · 2018-07-16T14:11:18Z

lib/definitions/livesync.d.ts

+	 * Checks if the current operation is in progress.
+	 * @param operationId - The identifier of the operation.
+	 */
+	isOperationInProgress(operationId: string): boolean;


Can't we handle the timeouts and the logging using just the sendDoSyncOperation promise without the generateOperationIdentifier and isOperationInProgress?

It is possible, but won't look good. As Promise has no property/method to check its state we have to attach to both resolve/reject and raise a local flag to indicate the state inside the interval function. It is again my personal preference to have the library provide the functionality.

DimitarTachev · 2018-07-16T14:14:12Z

lib/definitions/livesync.d.ts

+	/**
+	 * Closes the current socket connection.
+	 */
+	end(): void;


Isn't this the opposite of the connect method? It could be disconnect instead of end.

The main reason is the Socket API where the methods are actually connect and end. There is still a destroy method, but its used when an exception happened - reference.

DimitarTachev · 2018-07-16T14:16:21Z

lib/definitions/livesync.d.ts

+
+interface IAndroidLivesyncToolConfiguration {
+	/**
+	 * The application identifier.


Do we have a rule for commenting а self-documented code? I don't see any additional value from such comments.

DimitarTachev · 2018-07-16T14:32:37Z

lib/services/livesync/android-device-livesync-sockets-service.ts

+	}
+
+	public async refreshApplication(projectData: IProjectData, liveSyncInfo: ILiveSyncResultInfo): Promise<void> {
+		const canExecuteFastSync = !liveSyncInfo.isFullSync && !_.some(liveSyncInfo.modifiedFilesData,


extract the second part of the condition to a method for readability

DimitarTachev · 2018-07-16T15:19:28Z

lib/services/livesync/android-livesync-library.ts

+
+			setTimeout(() => {
+				if (!isResolved) {
+					isResolved = true;


isResolved -> isComplete in order to avoidisResolved = true when rejected?

DimitarTachev · 2018-07-16T15:25:10Z

lib/services/livesync/android-livesync-library.ts

+		for (const filePath of filePaths) {
+			if (this.$fs.getLsStats(filePath).isFile()) {
+				if (!this.$fs.exists(filePath)) {
+					this.$errors.fail(`${filePath} doesn't exist.`);


fail -> failWithoutHelp?

DimitarTachev · 2018-07-16T15:31:40Z

lib/services/livesync/android-livesync-library.ts

+					} else {
+						const error = this.checkConnectionStatus();
+						//TODO Destroy method added in node 8.0.0.
+						//when we depricate node 6.x uncoment the line belolw


Can't we use something like process.version and a semver check?

Don't think it`s worth the effort, given we will make older node versions unsupported not that far in the future.

DimitarTachev · 2018-07-16T15:40:55Z

lib/services/livesync/android-device-livesync-sockets-service.ts

+import { AndroidDeviceHashService } from "../../common/mobile/android/android-device-hash-service";
+import { DeviceLiveSyncServiceBase } from "./device-livesync-service-base";
+import { APP_FOLDER_NAME } from "../../constants";
+import { AndroidLivesyncTool } from "./android-livesync-library";


unify the class name and the file name

@rosen-vladimirov @Fatme which one do you prefer - changing the file name or changing the interfaces and variables?

I think changing the file name is better.

DimitarTachev · 2018-07-16T15:51:37Z

lib/services/livesync/android-livesync-library.ts

+
+	public end() {
+		if (this.socketConnection) {
+			this.socketConnection.end();


Can we also set socketConnection to null and allow a new connect call?

This will happen in the close event callback.

Fatme

After green build!

dtopuzov · 2018-07-20T20:20:27Z

test

Fatme · 2018-07-23T13:02:24Z

lib/services/livesync/android-device-livesync-sockets-service.ts

+		await this.doSync(liveSyncInfo, false);
+	}
+
+	private async doSync(liveSyncInfo: ILiveSyncResultInfo, doRefresh = false): Promise<IAndroidLivesyncSyncOperationResult> {


It is not a good practice to add boolean variable.
Use dictionary instead:

doSync(liveSyncInfo, { doRefresh: false })

Fatme · 2018-07-23T13:02:35Z

lib/services/livesync/android-device-livesync-sockets-service.ts

+		let result;
+		const operationId = this.livesyncTool.generateOperationIdentifier();
+
+		result = {operationId, didRefresh: true };


let result = {operationId, didRefresh: true };

…to {N} CLI and fix lint errors

rosen-vladimirov · 2018-08-07T06:28:23Z

Implements #3797

Natalia-Hristova added this to the 4.2.0 milestone Jul 16, 2018

Fatme reviewed Jul 16, 2018

View reviewed changes

DimitarTachev reviewed Jul 16, 2018

View reviewed changes

KristianDD force-pushed the kddimitrov/android-livesync-sockets branch from 1b5dfae to f81fc16 Compare July 18, 2018 09:00

Fatme approved these changes Jul 18, 2018

View reviewed changes

Fatme reviewed Jul 23, 2018

View reviewed changes

Fatme approved these changes Jul 23, 2018

View reviewed changes

KristianDD and others added 20 commits July 24, 2018 10:31

Initial integration of sockets tool

cf67b15

Generate hashes and init connection on beforeLiveSyncAction

27babb8

Await sync before restart. Update checksums.

d9f8cb8

chore: improve code

688e431

Add interval for syc wait feedback

4e3477e

Move https://github.com/NativeScript/nativescript-android-livesync-lib …

fb8af69

…to {N} CLI and fix lint errors

Rename android livesync-library and extract as public class

0a62b82

Regenerate npm-shrinkwrap file

23af849

Hook socket livesync with exit signals.

4e7e815

Cleanul livesync with sockets code.

91d3a31

fix: resovle ios livesync device service

dc600e1

fix: requirement for runtime version supporting sockets LS

645b527

docs: add documentation to the android-livesync-library

c0a1ec3

Fix PR comments

39f7db4

fix: app not restarted if in error activity

911b61f

chore: fix comments of PR

2d8039b

fix: unnecessary refresh before restart

e22a361

fix: timout not cleared on exit signals

294db1b

fix: debug doesn't await file trasfer end with socket implementation

d42cd90

chore: fix comments

5ffaba7

KristianDD force-pushed the kddimitrov/android-livesync-sockets branch from 09b4db5 to 5ffaba7 Compare July 24, 2018 07:33

KristianDD added 2 commits July 24, 2018 11:10

chore: update LS with sockets required runtime version

460b4a4

chore: update common lib

9273366

KristianDD merged commit 40201cb into master Jul 24, 2018

KristianDD deleted the kddimitrov/android-livesync-sockets branch November 21, 2018 14:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kddimitrov/android livesync sockets #3746

Kddimitrov/android livesync sockets #3746

KristianDD commented Jul 12, 2018 •

edited

Loading

dtopuzov commented Jul 13, 2018

Fatme Jul 16, 2018

Fatme Jul 16, 2018

DimitarTachev Jul 16, 2018

Fatme Jul 16, 2018

Fatme Jul 16, 2018

Fatme Jul 16, 2018

DimitarTachev left a comment

DimitarTachev Jul 16, 2018

DimitarTachev Jul 16, 2018

KristianDD Jul 17, 2018

DimitarTachev Jul 16, 2018

KristianDD Jul 17, 2018

DimitarTachev Jul 16, 2018 •

edited

Loading

DimitarTachev Jul 16, 2018

DimitarTachev Jul 16, 2018

DimitarTachev Jul 16, 2018

DimitarTachev Jul 16, 2018

KristianDD Jul 17, 2018 •

edited

Loading

DimitarTachev Jul 16, 2018

KristianDD Jul 17, 2018

Fatme Jul 17, 2018

DimitarTachev Jul 16, 2018

KristianDD Jul 17, 2018

Fatme left a comment

dtopuzov commented Jul 20, 2018

Fatme Jul 23, 2018

Fatme Jul 23, 2018

rosen-vladimirov commented Aug 7, 2018

Kddimitrov/android livesync sockets #3746

Kddimitrov/android livesync sockets #3746

Conversation

KristianDD commented Jul 12, 2018 • edited Loading

PR Checklist

What is the current behavior?

What is the new behavior?

dtopuzov commented Jul 13, 2018

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

DimitarTachev 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

Choose a reason for hiding this comment

DimitarTachev Jul 16, 2018 • 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

KristianDD Jul 17, 2018 • 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

Fatme left a comment

Choose a reason for hiding this comment

dtopuzov commented Jul 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosen-vladimirov commented Aug 7, 2018

KristianDD commented Jul 12, 2018 •

edited

Loading

DimitarTachev Jul 16, 2018 •

edited

Loading

KristianDD Jul 17, 2018 •

edited

Loading