Fix isValidNativeScript project #2630

rosen-vladimirov · 2017-03-21T18:20:39Z

The isValidNativeScript project method is used in Fusion. However the implementation is not correct. Fix it to have correct behavior.
In order to fix it, modify projectData.initialize method - there's obsolete code for migration from .tnsproject to package.json - we do not need it anymore, so remove it.
Also fix a case where failing builds do not fail the build process (incorrect argument passed to childProcess.spawnFromEvent).

Update documentation for PublicAPI.

Also in case CLI's executed in non-interactive terminal and there's no DEVELOPMENT_TEAM set, tns build ios --for-device fails with message "Console is not interactive and no default action specified" which does not give any info to the users.
So in this case add more descriptive error message.

yyosifov · 2017-03-23T09:27:16Z

lib/project-data.ts

 			let data: any = null;

-			if (this.$fs.exists(this.projectFilePath)) {
+			if (projectFilePath) {


how can this ever be undefined? Why did we remove the exists? It might just not be a valid project?

Thanks for pointing this, I've deleted it by mistake.

yyosifov · 2017-03-23T09:28:46Z

lib/project-data.ts

 				}

 				if (data) {
+					this.projectDir = projectDir;
+					this.projectName = this.$projectHelper.sanitizeName(path.basename(projectDir));
+					this.platformsDir = path.join(projectDir, constants.PLATFORMS_DIR_NAME);


projectName/platformsDir/appDirectoryPath/appResourcesDirectoryPath seem like "Computed properties" for me - they all are coming from the projectDir. Why not initialize only the project dir, and create getters for these properties so that we don't hit a bug if sometime we miss updating one of the computed ones? Do we have such a practice?

I like the idea, but there's a reason to set the properties here - we are accessing them in many, many places, so computing each of them every single time will require reading the directory, the package.json, etc. This will make a lot of I/O operations. I can cache the properties after first execution, but this will not work for Fusion, which is a long living process.
So I'll keep this code until I figure out a better way for handling this.

I think I didn't explain it correctly. What was my idea is that if you set the projectDir, all other properties - projectName, platformsDir, appDirectoryPath can be computed from the projectDir variable - without reading anything from the package.json

i.e. something like:

private _platformsDir: string; get platformsDir():boolean { if(!this._platformsDir) { this._platformsDir = path.join(this.projectDir, constants.PLATFORMS_DIR_NAME); } return this._platformsDir; }

so that we don't forget to initialize the platformsDir somewhere.

yyosifov · 2017-03-23T09:30:38Z

lib/project-data.ts

 		}
+
+		// This is the case when no project file found
+		this.$errors.fail("No project found at or above '%s' and neither was a --path specified.", projectDir || this.$options.path || path.resolve("."));


shall we add trace here, too? What bothers me is the three "or" statement, which will hide information from us on what exactly is the value of all of them.

yyosifov · 2017-03-23T09:33:09Z

lib/providers/livesync-provider.ts

 		}

-		return this.$platformService.getLatestApplicationPackageForDevice(platformData).packageName;
+		return this.$platformService.getLatestApplicationPackageForDevice(platformData, { isReleaseBuild: buildConfig.release }).packageName;


Do you think we can rename from isReleaseBuild to release so that it's more consistent with the IBuildConfig?

yyosifov · 2017-03-23T09:33:20Z

lib/providers/livesync-provider.ts

 		}

-		return this.$platformService.getLatestApplicationPackageForDevice(platformData).packageName;
+		return this.$platformService.getLatestApplicationPackageForDevice(platformData, { isReleaseBuild: buildConfig.release }).packageName;


Do you think we can directly pass on the buildConfig property, so that we have it in handy in case we need other property in the future? This would be only convenient if we have easy way to access the BuildConfig or create it at handy

yyosifov · 2017-03-23T09:37:29Z

lib/services/android-project-service.ts

-					`${projectData.projectName}-release.apk`
-				],
+				getValidPackageNames: (buildOptions: { isReleaseBuild?: boolean, isForDevice?: boolean }): string[] => {
+					if (buildOptions.isReleaseBuild) {


Can be rewritten as:

const buildMode = buildOptions.isReleaseBuild ? "release" : "debug"; // or constants return [ `${packageName}-${buildMode}.apk`, `${projectData.projectName}-${buildMode}.apk` ];

yyosifov · 2017-03-23T09:38:18Z

lib/services/android-project-service.ts

@@ -262,7 +269,7 @@ export class AndroidProjectService extends projectServiceBaseLib.PlatformProject
 			await this.spawn(gradleBin,
 				buildOptions,
 				{ stdio: buildConfig.buildOutputStdio || "inherit", cwd: this.getPlatformData(projectData).projectRoot },
-				{ emitOptions: { eventName: constants.BUILD_OUTPUT_EVENT_NAME }, throwError: false });
+				{ emitOptions: { eventName: constants.BUILD_OUTPUT_EVENT_NAME }, throwError: true });


yyosifov · 2017-03-23T09:39:53Z

lib/services/android-project-service.ts

@@ -426,6 +433,10 @@ export class AndroidProjectService extends projectServiceBaseLib.PlatformProject
 	}

 	public async cleanProject(projectRoot: string, options: string[], projectData: IProjectData): Promise<void> {
+		// In case options are not passed, we'll use the default ones for cleaning the project.
+		// In case we do not do this, we'll required android-23 (default in build.gradle) for cleaning the project.
+		options = options.length === 0 ? this.getBuildOptions({ release: false }, projectData) : options;


Do you think it would be a good idea to "extend"/"merge" the options passed with the "default options" which the getBuildOptions returns? So that we don't end up with fewer options than we need?

In fact the options to cleanProject are passed only at a single place and they are the result of this.getBuildOptions({ release: false }, projectData), so instead of applying some merge mechanism, I've decided to remove the options from the method and always generate them here.

yyosifov · 2017-03-23T09:40:33Z

lib/services/android-project-service.ts

@@ -426,6 +433,10 @@ export class AndroidProjectService extends projectServiceBaseLib.PlatformProject
 	}

 	public async cleanProject(projectRoot: string, options: string[], projectData: IProjectData): Promise<void> {
+		// In case options are not passed, we'll use the default ones for cleaning the project.
+		// In case we do not do this, we'll required android-23 (default in build.gradle) for cleaning the project.


small typo: "we'll required" to "we'll require" I guess?

Mitko-Kerezov

After fixing minor comments

Mitko-Kerezov · 2017-03-23T08:55:16Z

lib/definitions/platform.d.ts

@@ -143,28 +143,30 @@ interface IPlatformService extends NodeJS.EventEmitter {
 	/**
 	 * Returns information about the latest built application for device in the current project.
 	 * @param {IPlatformData} platformData Data describing the current platform.
+	 * @param {any} buildOptions Defines if the build is for release configuration.


Mitko-Kerezov · 2017-03-23T08:55:34Z

lib/definitions/platform.d.ts


 	/**
 	 * Returns information about the latest built application for simulator in the current project.
 	 * @param {IPlatformData} platformData Data describing the current platform.
+	 * @param {any} buildOptions Defines if the build is for release configuration.


any again

Mitko-Kerezov · 2017-03-23T08:56:12Z

lib/project-data.ts

 					data = fileContent[this.$staticConfig.CLIENT_NAME_KEY_IN_PROJECT_FILE];
 				} catch (err) {
-					this.$errors.fail({
-						formatStr: "The project file %s is corrupted." + EOL +
+					this.$errors.failWithoutHelp(


You can pass a single template string as an argument here instead of concatenating template strings with regular ones

I'll use three template strings in order to keep the readability of the code and keep the formatting of the message.

Mitko-Kerezov · 2017-03-23T08:57:58Z

lib/project-data.ts

 			let data: any = null;

-			if (this.$fs.exists(this.projectFilePath)) {
+			if (projectFilePath) {


IMO it makes more sense to invert the if statement and just fail if projectFilePath isn't set. It'd make the code more readable I think, because right now the majority of the working code is inside the if statement

I would like to fail in case projectFilePath does not exist or in case there's no data in the file. There are two cases where we'll fail with the same error (that a project is not found), that's why I've left the throw statement at the end of the method.

yyosifov · 2017-03-23T11:52:47Z

lib/services/project-changes-service.ts

 		private $fs: IFileSystem) {
 	}

 	public get currentChanges(): IProjectChangesInfo {
 		return this._changesInfo;
 	}

-	public checkForChanges(platform: string, projectData: IProjectData): IProjectChangesInfo {
+	public checkForChanges(platform: string, projectData: IProjectData, buildOptions: IProjectChangesOptions): IProjectChangesInfo {


Weird naming for me: "buildOptions for ProjectChangesOptions"

yyosifov · 2017-03-23T11:54:07Z

lib/services/project-service.ts

@@ -73,11 +73,10 @@ export class ProjectService implements IProjectService {
 	public isValidNativeScriptProject(pathToProject?: string): boolean {


I was wondering, do we want to return some errors if we can identify. I mean - OK, it's not a valid NativeScript project but what's the actual issue? Can the user repair his project to be valid - in case he can, I'd suggest we somehow give him a hint what needs to be fixed? What do you think?

This method is created for Fusion, where when the user selects a folder, we try to check if this is a valid project.
All NativeScript CLI users will see the message from initializeProjectData method.

rosen-vladimirov · 2017-03-23T20:57:21Z

Ping @yyosifov , @Mitko-Kerezov - comments are fixed

Mitko-Kerezov

After fixing minor cosmetic comment

Mitko-Kerezov · 2017-03-24T08:16:19Z

lib/services/ios-debug-service.ts

@@ -93,10 +93,10 @@ class IOSDebugService implements IDebugService {
 		}
 	}

-	private async emulatorDebugBrk(projectData: IProjectData, shouldBreak?: boolean): Promise<void> {
+	private async emulatorDebugBrk(projectData: IProjectData, buildConfig: IBuildConfig, shouldBreak?: boolean, ): Promise<void> {


Gratuitous comma at the end of args list

The isValidNativeScript project method is used in Fusion. However the implementation is not correct. Fix it to have correct behavior. In order to fix it, modify projectData.initialize method - there's obsolete code for migration from .tnsproject to package.json - we do not need it anymore, so remove it. Also fix a case where failing builds do not fail the build process (incorrect argument passed to childProcess.spawnFromEvent). Update documentation for PublicAPI.

Also in case CLI's executed in non-interactive terminal and there's no DEVELOPMENT_TEAM set, tns build ios --for-device fails with message "Console is not interactive and no default action specified" which does not give any info to the users. So in this case add more descriptive error message.

When we try to get the latest application package for device/emulator, we check the build output directory and get latest available .apk/.ipa. However this is not always correct as sometimes we do not build the app. Consider the following case: * tns build android --release * tns build android * tns build android --release At the last point, the build will not be executed, as there are no changes. However the last built .apk is from the debug build (we have release .apk, but it's older). So in case we try to get the last build output from last operation, CLI will return the debug.apk Fix this by checking the current build configuration and get the latest result by using it. For iOS respect the expected output - is it for device or for simulator as the output is different.

When CLI is required as library, the `$options` dependency is not populated. However the projectChangesService relies on it in order to determine if the project should be prepared/built. When CLI is required as library and you change only the build configuration (debug/release), the project is not rebuilt. However when you use the CLI and try the same, a new full build is triggered. Fix this by parsing required boolean flags to projectChangesService and exclude `$options` from its implementation. This way local builds will work in the same manner both from command line and when required as library. Steps to reproduce the problem: * use CLI as lib * create local build for android in debug configuration * create local build for android in release configuration - you'll notice gradle clean is not called at all and also in the `<project dir>/platforms/android/build/outputs/apks/` there are both debug and release apks. This should never happen when changing configurations.

Pass the whole buildConfig object when getting the path to the last build output. This required changes in debug services and TestExecutionService. Also change the args of cleanProject method - the `options` passed to `gradle` are now determined in the method itself instead of passing them from the caller. This way we'll not require "android-23" (the default one set in build.gradle) when we miss to pass the required options.

rosen-vladimirov added the bug label Mar 21, 2017

rosen-vladimirov self-assigned this Mar 21, 2017

rosen-vladimirov force-pushed the vladimirov/public-api-fixes branch from bc52ef9 to 6747e54 Compare March 21, 2017 18:39

yyosifov reviewed Mar 23, 2017

View reviewed changes

rosen-vladimirov mentioned this pull request Mar 23, 2017

Build continues after gradle build fails #2639

Closed

Mitko-Kerezov approved these changes Mar 23, 2017

View reviewed changes

yyosifov reviewed Mar 23, 2017

View reviewed changes

rosen-vladimirov force-pushed the vladimirov/public-api-fixes branch from 6ed8cfd to aca4275 Compare March 23, 2017 20:55

Mitko-Kerezov approved these changes Mar 24, 2017

View reviewed changes

rosen-vladimirov and others added 5 commits March 24, 2017 10:40

rosen-vladimirov force-pushed the vladimirov/public-api-fixes branch from aca4275 to c5b1129 Compare March 24, 2017 08:41

rosen-vladimirov merged commit ddd8328 into master Mar 24, 2017

rosen-vladimirov deleted the vladimirov/public-api-fixes branch March 24, 2017 09:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix isValidNativeScript project #2630

Fix isValidNativeScript project #2630

rosen-vladimirov commented Mar 21, 2017 •

edited

Loading

yyosifov Mar 23, 2017

rosen-vladimirov Mar 23, 2017

yyosifov Mar 23, 2017 •

edited

Loading

rosen-vladimirov Mar 23, 2017

yyosifov Mar 24, 2017

yyosifov Mar 23, 2017 •

edited

Loading

yyosifov Mar 23, 2017

yyosifov Mar 23, 2017 •

edited

Loading

yyosifov Mar 23, 2017 •

edited

Loading

yyosifov Mar 23, 2017

yyosifov Mar 23, 2017

rosen-vladimirov Mar 23, 2017

yyosifov Mar 23, 2017

Mitko-Kerezov left a comment

Mitko-Kerezov Mar 23, 2017

Mitko-Kerezov Mar 23, 2017

Mitko-Kerezov Mar 23, 2017

rosen-vladimirov Mar 23, 2017

Mitko-Kerezov Mar 23, 2017

rosen-vladimirov Mar 23, 2017

yyosifov Mar 23, 2017

yyosifov Mar 23, 2017

rosen-vladimirov Mar 23, 2017

rosen-vladimirov commented Mar 23, 2017

Mitko-Kerezov left a comment

Mitko-Kerezov Mar 24, 2017

		@@ -73,11 +73,10 @@ export class ProjectService implements IProjectService {
		public isValidNativeScriptProject(pathToProject?: string): boolean {

Fix isValidNativeScript project #2630

Fix isValidNativeScript project #2630

Conversation

rosen-vladimirov commented Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yyosifov Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yyosifov Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yyosifov Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

yyosifov Mar 23, 2017 • 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

Mitko-Kerezov 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

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 Mar 23, 2017

Mitko-Kerezov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosen-vladimirov commented Mar 21, 2017 •

edited

Loading

yyosifov Mar 23, 2017 •

edited

Loading

yyosifov Mar 23, 2017 •

edited

Loading

yyosifov Mar 23, 2017 •

edited

Loading

yyosifov Mar 23, 2017 •

edited

Loading