fix: find correct apk name when --release passed #3419

Plamen5kov · 2018-03-05T09:15:25Z

problem
The CLI can't find .apk file to install on passed --release option. Read more in this issue

solution
respect --release option when generating apk find path

rosen-vladimirov

Using $options in service will make it unusable when CLI is used as a library. In this case, this will break local release builds in Sidekick, as the $options is not populated when CLI is not used on the command line.
Instead of using $options, the getDeviceBuildOutputPath should accept the build configuration. This requires changing the deviceBuildOutputPath to become a method and to be recalculated based on the passed options. The only place where deviceBuildOutputPath is used is here - you can directly pass the buildConfig object to the method.

Plamen5kov · 2018-03-06T06:35:43Z

@rosen-vladimirov thank's for explaining. Will fix it shortly.

Fatme · 2018-03-06T12:51:11Z

lib/services/platform-service.ts

@@ -431,15 +431,15 @@ export class PlatformService extends EventEmitter implements IPlatformService {
 		this.$fs.writeJson(buildInfoFile, buildInfo);
 	}

-	public async shouldInstall(device: Mobile.IDevice, projectData: IProjectData, outputPath?: string): Promise<boolean> {
+	public async shouldInstall(device: Mobile.IDevice, projectData: IProjectData, release: boolean, outputPath?: string): Promise<boolean> {


release param is not used in method

my bad, thank you

Fatme · 2018-03-06T12:51:35Z

lib/services/platform-service.ts

 		const platform = device.deviceInfo.platform;
 		if (!(await device.applicationManager.isApplicationInstalled(projectData.projectId))) {
 			return true;
 		}

 		const platformData = this.$platformsData.getPlatformData(platform, projectData);
 		const deviceBuildInfo: IBuildInfo = await this.getDeviceBuildInfo(device, projectData);
-		const localBuildInfo = this.getBuildInfo(platform, platformData, { buildForDevice: !device.isEmulator }, outputPath);
+		const localBuildInfo = this.getBuildInfo(platform, platformData, { buildForDevice: !device.isEmulator, release: false }, outputPath);


Should be release: release

Fatme · 2018-03-06T12:54:06Z

lib/definitions/project.d.ts

@@ -140,6 +140,7 @@ interface IPlatformProjectServiceBase {

 interface IBuildForDevice {
 	buildForDevice: boolean;
+	release: boolean;


extends IRelease

The idea of IBuildForDevice interface is to be used whenever buildForDevice option is required.
Instead of using the IBuildForDevice interface, I suggest creating a new one that extends both IBuildForDevice and IRelease. The other option is to rename the current one, as the current properties are not required only for IBuildForDevice implementation.

Fatme · 2018-03-06T12:56:38Z

lib/definitions/platform.d.ts

@@ -76,7 +76,7 @@ interface IPlatformService extends NodeJS.EventEmitter {
 	 * @param {string} @optional outputPath Directory containing build information and artifacts.
 	 * @returns {Promise<boolean>} true indicates that the application should be installed.
 	 */
-	shouldInstall(device: Mobile.IDevice, projectData: IProjectData, outputPath?: string): Promise<boolean>;
+	shouldInstall(device: Mobile.IDevice, projectData: IProjectData, release: boolean, outputPath?: string): Promise<boolean>;


It's a bad practice to use boolean parameters.

Why is it a bad practice to use boolean parameters? Please elaborate.

Passing boolean as an argument makes the call to the method unreadable.
For the current case, when you call the method, you'll write:

await this.$platformService.shouldInstall(deviceInstance, projectData, false);

You have no idea what does this false do. When you try to read the flow of the code, you cannot tell why the value false is passed. In such cases it is recommended to pass object with boolean property, for example:

shouldInstall(device: Mobile.IDevice, projectData: IProjectData, buildOptions: IRelease, outputPath?: string): Promise<boolean>;

and the call will be:

await this.$platformService.shouldInstall(deviceInstance, projectData, { release: false });

This way there's no need to read the implementation of the method.

@rosen-vladimirov thanks, I can see now why using a boolean parameter could be bad.

rosen-vladimirov · 2018-03-06T14:01:04Z

lib/definitions/platform.d.ts

@@ -76,7 +76,7 @@ interface IPlatformService extends NodeJS.EventEmitter {
 	 * @param {string} @optional outputPath Directory containing build information and artifacts.
 	 * @returns {Promise<boolean>} true indicates that the application should be installed.
 	 */
-	shouldInstall(device: Mobile.IDevice, projectData: IProjectData, outputPath?: string): Promise<boolean>;
+	shouldInstall(device: Mobile.IDevice, projectData: IProjectData, release: boolean, outputPath?: string): Promise<boolean>;


Passing boolean as an argument makes the call to the method unreadable.
For the current case, when you call the method, you'll write:

await this.$platformService.shouldInstall(deviceInstance, projectData, false);

You have no idea what does this false do. When you try to read the flow of the code, you cannot tell why the value false is passed. In such cases it is recommended to pass object with boolean property, for example:

shouldInstall(device: Mobile.IDevice, projectData: IProjectData, buildOptions: IRelease, outputPath?: string): Promise<boolean>;

and the call will be:

await this.$platformService.shouldInstall(deviceInstance, projectData, { release: false });

This way there's no need to read the implementation of the method.

rosen-vladimirov · 2018-03-06T14:01:37Z

lib/definitions/platform.d.ts

@@ -255,7 +255,7 @@ interface IPlatformData {
 	projectRoot: string;
 	normalizedPlatformName: string;
 	appDestinationDirectoryPath: string;
-	deviceBuildOutputPath: string;
+	deviceBuildOutputPath(options: IRelease): string;


I believe we should rename this property to getDeviceBuildOutputPath as it is a better name for method.

rosen-vladimirov · 2018-03-06T14:04:38Z

lib/definitions/project.d.ts

@@ -140,6 +140,7 @@ interface IPlatformProjectServiceBase {

 interface IBuildForDevice {
 	buildForDevice: boolean;
+	release: boolean;


The idea of IBuildForDevice interface is to be used whenever buildForDevice option is required.
Instead of using the IBuildForDevice interface, I suggest creating a new one that extends both IBuildForDevice and IRelease. The other option is to rename the current one, as the current properties are not required only for IBuildForDevice implementation.

Fatme · 2018-03-07T10:02:45Z

lib/services/platform-service.ts

@@ -431,15 +431,15 @@ export class PlatformService extends EventEmitter implements IPlatformService {
 		this.$fs.writeJson(buildInfoFile, buildInfo);
 	}

-	public async shouldInstall(device: Mobile.IDevice, projectData: IProjectData, outputPath?: string): Promise<boolean> {
+	public async shouldInstall(device: Mobile.IDevice, projectData: IProjectData, isRelease: IRelease, outputPath?: string): Promise<boolean> {


I suggest to reuse IBuildConfig interface as we pass here buildConfig https://github.com/NativeScript/nativescript-cli/pull/3419/files#diff-513fe963f5d821a1cdb167ea91d70f1cR335

What will changing the interface from IRelease to IBuildConfig achieve?

We discussed and agreed to use IRelease interface, only isRelease name sounds like a boolean variable and suggest to rename it to release: IRelease

rosen-vladimirov

After green build

Plamen5kov requested a review from rosen-vladimirov March 5, 2018 09:15

Plamen5kov self-assigned this Mar 5, 2018

Plamen5kov added this to the 4.0.0 milestone Mar 5, 2018

Plamen5kov added the in progress label Mar 5, 2018

rosen-vladimirov suggested changes Mar 5, 2018

View reviewed changes

Fatme suggested changes Mar 6, 2018

View reviewed changes

rosen-vladimirov suggested changes Mar 6, 2018

View reviewed changes

Fatme reviewed Mar 7, 2018

View reviewed changes

Fatme approved these changes Mar 7, 2018

View reviewed changes

rosen-vladimirov approved these changes Mar 7, 2018

View reviewed changes

Plamen5kov added 4 commits March 8, 2018 10:25

fix: find correct apk name when --release passed

9fe5509

refactor: don't use static options

e437bd3

refactor: update after review

d5c4eed

chore: rename variable

40f5074

Plamen5kov force-pushed the plamen5kov/fix-apk-search-dir branch from 7204eed to 40f5074 Compare March 8, 2018 08:26

Plamen5kov merged commit 69bc287 into master Mar 8, 2018

Plamen5kov deleted the plamen5kov/fix-apk-search-dir branch March 8, 2018 09:03

Uh oh!

fix: find correct apk name when --release passed #3419

fix: find correct apk name when --release passed #3419

Uh oh!

Conversation

Plamen5kov commented Mar 5, 2018

Uh oh!

rosen-vladimirov left a comment

Choose a reason for hiding this comment

Uh oh!

Plamen5kov commented Mar 6, 2018

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!

rosen-vladimirov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!