Chnged: `tns run` command to watch for changes by default #2374

tzraikov · 2016-12-27T10:17:28Z

Changes the run command behavior to watch for changes by executing livesync.
Changes the livesync behavior to watch for changes in all files. The project-changes-service detects whether the change requires a full build or files can be livesynced.
The livesync state is preserved between calls, so no extra files will be transferred on second run.

Fixes: #1366, #2276, #2277, #2303, #2315, #2330, #2341, and most parts of #2153

The new prepare, build, deploy, run, debug and livesync commands behavior is described here: https://github.com/NativeScript/nativescript-cli/wiki/Workflow-commands-spec

It is based on the one described in this issue:
#475

Currently there are two issues not related with livesync:

the snapshot plugin executes in debug and modifies file times. This breaks the livesync logic. The change should be in snapshot module.
changing the app id in package.json doesn't work on android because the gradle script uses the app.gradle file directly from App_Resources folder.

Currently livesync doesn't work when adding/removing project dependencies. We should add this in our documentation.

DO NOT MERGE BEFORE telerik/mobile-cli-lib#863 is merged in mobile-cli-lib

rosen-vladimirov · 2016-12-27T14:38:09Z

lib/services/livesync/livesync-service.ts

@@ -3,11 +3,9 @@ import * as helpers from "../../common/helpers";
 import * as path from "path";
 import * as semver from "semver";
 import * as fiberBootstrap from "../../common/fiber-bootstrap";
-
-let gaze = require("gaze");
+let choki = require("chokidar");


Is there a reason to replace gaze with chokidar?

Also it's not added to package.json, so it will not be installed on any machine.

Yes, there is a reason. I observed that gaze is quite slow and unstable when watching for a large number of files, while chokidar handles this scenario fast and reliable. Chokidar is the preferred watching tool used in webpack.

I added the corresponding entry in package.json

rosen-vladimirov · 2016-12-27T14:43:40Z

lib/services/livesync/livesync-service.ts

-		this.$processService.attachToProcessExitSignals(this, () => gazeWatcher.close());
+		this.$processService.attachToProcessExitSignals(this, () => {
+			watcher.unwatch(pattern);
+			process.exit();


Why do you need process.exit call here? The attachToProcessExitSignals means that it's already in process of exiting current process.

I was not able to stop the process without calling process.exit. Maybe there is something specific in chokidar preventing the process from exiting. I didn't observed any side effects while testing this code. Watching for changes in all files is important in order to support predictable livesync. So I am open for suggestions.

Just found a way to stop the watcher without process.exit. Changed the code.

rosen-vladimirov · 2016-12-28T12:52:02Z

lib/options.ts

-			chrome: { type: OptionType.Boolean }
+			chrome: { type: OptionType.Boolean },
+			clean: { type: OptionType.Boolean },
+			watch: { type: OptionType.Boolean, default: true },


no need of comma (,) at the end

Technically, yes. Someone could argue that this way it is easier to add new entries, however different languages, different styles.. removed the comma.

rosen-vladimirov · 2016-12-28T13:12:04Z

lib/services/livesync/platform-livesync-service.ts

+				let localToDevicePaths:Mobile.ILocalToDevicePathData[] = null;
+				let isFullSync = false;
+				if (this.$options.clean || this.$projectChangesService.currentChanges.changesRequireBuild) {
+					this.$platformService.buildPlatform(this.liveSyncData.platform, { buildForDevice: !device.isEmulator }).wait();


In case you have two (or more) devices attached, you'll build your project for each of them, while in fact even a single build may be enough.
However this can be handled in a separate PR.

Correct! Changed the logic to build only when necessary. 👍

rosen-vladimirov · 2016-12-28T13:39:23Z

lib/services/platform-service.ts

 				excludedDirs.push(constants.TNS_MODULES_FOLDER_NAME);
 			}

 			this.$projectFilesManager.processPlatformSpecificFiles(directoryPath, platform, excludedDirs);

-			if (changeInfo.configChanged || changeInfo.modulesChanged) {
+			if (!changesInfo || (changesInfo.configChanged || changesInfo.modulesChanged)) {


no need of additional (, just:

if (!changesInfo || changesInfo.configChanged || changesInfo.modulesChanged) {

correct! 👍

rosen-vladimirov · 2016-12-28T14:17:57Z

lib/services/platform-service.ts

+		return path.join(deviceRootPath, buildInfoFileName);
+	}
+
+	private getDeviceBuildInfo(device: Mobile.IDevice): IBuildInfo {


this method has async operation, so it should return IFuture<IBuildInfo> instead of IBuildInfo. When there's async code in the body, you should wrap it in a future:

return (() => { .... }).future<IBuildInfo>()();

Changed the method to use IFuture. 👍

rosen-vladimirov · 2016-12-28T15:11:00Z

lib/services/platform-service.ts

+
+	public shouldInstall(device: Mobile.IDevice): boolean {
+		if(this.$options.release) {
+			return true;


what's the logic to always install the app in case --release is specified?

Humm, just wanted to ensure that everything is ok in release. In case if there are some artefacts from debug. It shouldn't be necessary.

Removed the code.

rosen-vladimirov · 2016-12-28T16:22:09Z

lib/services/project-changes-service.ts

+				if (processFunc) {
+					this._newFiles ++;
+					let filePathRelative = path.relative(this.$projectData.projectDir, filePath);
+					let that = this;


I believe you do not need that variable here, you can pass this directly.

Removed the variable. 👍

rosen-vladimirov · 2016-12-28T18:49:50Z

I've tested the current implementation for Android and there's some strange behavior. It looks like no matter of the change I made, a package.json is always transferred to the device. This causes double restart of the application during livesync. You can check this video where the issue is visible.

rosen-vladimirov · 2016-12-28T19:03:14Z

Based on the code and related issues in the descriptions, I assume the current PR should allow adding new dependencies in package.json during tns run android (while file system watcher is working). I've tried the following:

In terminal 1: tns run android
After it finishes, I've opened terminal 2 and I've tried npm install --save lodash. Before it finished, I received an error in terminal 1:

Executing before-prepare hook from D:\Work\nativescript-cli\scratch\app2303-1\hooks\before-prepare\nativescript-dev-android-snapshot.js
Preparing project...
Project successfully prepared (android)
Executing after-prepare hook from D:\Work\nativescript-cli\scratch\app2303-1\hooks\after-prepare\nativescript-dev-android-snapshot.js
Building project...

:config phase:  createDefaultIncludeFiles
        +found plugins: nativescript-barcodescanner
        +found plugins: tns-core-modules-widgets

:config phase:  createPluginsConfigFile
         Creating product flavors include.gradle file in D:\Work\nativescript-cli\scratch\app2303-1\platforms\android/configurations folder...

:config phase:  pluginExtend
        +applying configuration from: D:\Work\nativescript-cli\scratch\app2303-1\platforms\android\configurations\include.gradle
        +applying configuration from: D:\Work\nativescript-cli\scratch\app2303-1\platforms\android\configurations\nativescript-barcodescanner\include.gradle
        +applying configuration from: D:\Work\nativescript-cli\scratch\app2303-1\platforms\android\configurations\tns-core-modules-widgets\include.gradle

:config phase:  copyAarDependencies
> Configuring > 0/3 projects > root projectError: EPERM: operation not permitted, lstat 'D:\Work\nativescript-cli\scratch\app2303-1\node_modules\lodash'
1/3 projects
vladimirov@VLADIMIROV  /d/Work/nativescript-cli/scratch/app2303-1 (raikov/fix-2303c)
Observed package id 'build-tools;21.1.1' in inconsistent location 'D:\Program Files\android_sdk\build-tools\build-tools-21.1.1' (Expected 'D:\Program Files\android_sdk\build-tools\21.1.1')

FAILURE: Build failed with an exception.

* Where:
Build file 'D:\Work\nativescript-cli\scratch\app2303-1\platforms\android\build.gradle' line: 491

* What went wrong:
A problem occurred evaluating root project 'app23031'.
> Could not read path 'D:\Work\nativescript-cli\scratch\app2303-1\node_modules\lodash'.

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output.

BUILD FAILED

Total time: 6.524 secs

Should this work?

tzraikov · 2016-12-29T09:16:13Z

Regarding the scond rebuild on android, yes I confirm the issue. Like I pointed, it is caused by the android snapshot plugin. It modifies last modified times for different files explicitly every time it is executed (regardless if this is debug or release). I am not sure why it does this. It breaks the livesync watching logic. My personal opinion is that it should be corrected in android snapshot plugin. It should be able to work without changing file times explicitly.

You shouldn't see this glitch if you remove the hooks related with android snapshot plugin from your project.

tzraikov · 2016-12-29T09:21:04Z

No, currently adding dependencies while watching for changes will not work. This is caused by the fact that livesync syncs/rebuilds on every change. When adding a new dependency you are adding a lot of files at the same time. This will lead to unpredictable behaviour. In some cases it will cause endless sync/rebuild on every file change, in other cases it will break. I was not able to find an easy solution for this scenario. We should state this explicitly in our documentation.

vchimev · 2017-01-03T17:07:12Z

NOTE: Verify that this PR does not broke the case below:

Cannot run livesync with ios simulator #2267 (comment)

Implemented pt redictable livesync Implement run command to use livesync project changes info as a service

tzraikov force-pushed the raikov/fix-2303c branch 6 times, most recently from 8a98e65 to f788f7e Compare December 28, 2016 09:27

tzraikov self-assigned this Dec 28, 2016

tzraikov force-pushed the raikov/fix-2303c branch 2 times, most recently from 6c3e780 to b423d3b Compare December 28, 2016 12:05

rosen-vladimirov suggested changes Dec 28, 2016

View reviewed changes

tzraikov force-pushed the raikov/fix-2303c branch 7 times, most recently from 818d805 to 1c81407 Compare December 29, 2016 09:10

tzraikov force-pushed the raikov/fix-2303c branch from 1c81407 to 81d4ca4 Compare December 29, 2016 10:26

vchimev mentioned this pull request Jan 3, 2017

Cannot run livesync with ios simulator #2267

Closed

tzraikov force-pushed the raikov/fix-2303c branch 3 times, most recently from 40d525c to c8d5a1a Compare January 9, 2017 09:50

Fixed: Changes in package.json are not livesynced

b35259a

Implemented pt redictable livesync Implement run command to use livesync project changes info as a service

tzraikov force-pushed the raikov/fix-2303c branch from c8d5a1a to b35259a Compare January 9, 2017 14:01

tzraikov merged commit 57ddffe into master Jan 9, 2017

pkoleva deleted the raikov/fix-2303c branch January 18, 2017 07:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chnged: `tns run` command to watch for changes by default #2374

Chnged: `tns run` command to watch for changes by default #2374

tzraikov commented Dec 27, 2016 •

edited

Loading

rosen-vladimirov Dec 27, 2016

rosen-vladimirov Dec 28, 2016

tzraikov Dec 29, 2016

rosen-vladimirov Dec 27, 2016

tzraikov Dec 29, 2016

tzraikov Dec 29, 2016

rosen-vladimirov Dec 28, 2016

tzraikov Dec 29, 2016

rosen-vladimirov Dec 28, 2016

tzraikov Dec 29, 2016

rosen-vladimirov Dec 28, 2016

tzraikov Dec 29, 2016

rosen-vladimirov Dec 28, 2016

tzraikov Dec 29, 2016

rosen-vladimirov Dec 28, 2016

tzraikov Dec 29, 2016

tzraikov Dec 29, 2016

rosen-vladimirov Dec 28, 2016

tzraikov Dec 29, 2016

rosen-vladimirov commented Dec 28, 2016

rosen-vladimirov commented Dec 28, 2016

tzraikov commented Dec 29, 2016

tzraikov commented Dec 29, 2016

vchimev commented Jan 3, 2017 •

edited by tzraikov

Loading

Chnged: tns run command to watch for changes by default #2374

Chnged: tns run command to watch for changes by default #2374

Conversation

tzraikov commented Dec 27, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosen-vladimirov commented Dec 28, 2016

rosen-vladimirov commented Dec 28, 2016

tzraikov commented Dec 29, 2016

tzraikov commented Dec 29, 2016

vchimev commented Jan 3, 2017 • edited by tzraikov Loading

Chnged: `tns run` command to watch for changes by default #2374

Chnged: `tns run` command to watch for changes by default #2374

tzraikov commented Dec 27, 2016 •

edited

Loading

vchimev commented Jan 3, 2017 •

edited by tzraikov

Loading