Kddimitrov/aab build #4084

KristianDD · 2018-11-01T14:18:45Z

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.

What is the current behavior?

You can't generate .aab file.

What is the new behavior?

You can generate .aab file using tns build android --aab.

Fixes/Implements/Closes #4068

Fatme · 2018-11-01T14:35:16Z

lib/services/android-project-service.ts

 		const gradleArgs = this.getGradleBuildOptions(buildConfig, projectData);
+		const baseTask = buildConfig.androidBundle ? "bundle" : "assemble";
+		const platformData = this.getPlatformData(projectData);
+		const outputPath = buildConfig.androidBundle ? platformData.bundleBuildOutputPath : platformData.deviceBuildOutputPath;


Can we use this._platformData here?

No, because we can't mock it inside tests - _platformData is private. Initially I used this._platformData.

And _platformData is not used anywhere else inside the service. Everywhere we use this.getPlatformData, but we should change that or at least cache the platformDatabased on the projectData.

Fatme · 2018-11-01T14:36:24Z

lib/services/android-project-service.ts

 		if (this.$logger.getLevel() === "TRACE") {
 			gradleArgs.unshift("--stacktrace");
 			gradleArgs.unshift("--debug");
 		}
 		if (buildConfig.release) {
-			gradleArgs.unshift("assembleRelease");
+			task = baseTask + "Release";


task = `${baseTask}Release`

Shouldn't we have a linter rule if we won't use string concatenation at all?

Fatme · 2018-11-01T14:36:57Z

lib/services/android-project-service.ts

 		} else {
-			gradleArgs.unshift("assembleDebug");
+			task = baseTask + "Debug";


task = `${baseTask}Debug`

Fatme · 2018-11-01T14:39:17Z

lib/services/platform-service.ts

@@ -256,7 +256,11 @@ export class PlatformService extends EventEmitter implements IPlatformService {
 		return true;
 	}

-	public async validateOptions(provision: true | string, teamId: true | string, projectData: IProjectData, platform?: string): Promise<boolean> {
+	public async validateOptions(provision: true | string, teamId: true | string, projectData: IProjectData, platform?: string, aab?: boolean): Promise<boolean> {
+		if (platform && this.$mobileHelper.isiOSPlatform(platform) && aab) {


if (platform && !this.$mobileHelper.isAndroidPlatform(platform) && aab)

is better because if we add new platform the above check will be fine.

Fatme · 2018-11-01T14:48:44Z

lib/constants.ts

@@ -241,3 +243,8 @@ export class BundleValidatorMessages {
 	public static MissingBundlePlugin = "Passing --bundle requires a bundling plugin. No bundling plugin found or the specified bundling plugin is invalid.";
 	public static NotSupportedVersion = `The NativeScript CLI requires nativescript-dev-webpack %s or later to work properly. After updating nativescript-dev-webpack you need to ensure "webpack.config.js" file is up to date with the one in the new version of nativescript-dev-webpack. You can automatically update it using "./node_modules/.bin/update-ns-webpack --configs" command.`;
 }
+
+export class AndroidBundleValidatorMessages {
+	public static AAB_NOT_SUPPORTED_BY_COMMNAND_MESSAGE = "This command does not support --aab (Android Application Bundle) parameter.";


NOT_SUPPORTED_AAB_OPTION maybe?

I think AAB_NOT_SUPPORTED_BY_COMMNAND_MESSAGE is better at describing the message. If you see in constants NOT_SUPPORTED_AAB_OPTION and decide to use it in a service the error message will look strange for sidekick for example.

Fatme · 2018-11-01T14:49:29Z

lib/constants.ts

+
+export class AndroidBundleValidatorMessages {
+	public static AAB_NOT_SUPPORTED_BY_COMMNAND_MESSAGE = "This command does not support --aab (Android Application Bundle) parameter.";
+	public static RUNTIME_VERSION_TOO_LOW = "Android Application Bundle (--aab) option requires NativeScript Android Runtime (tns-android) version %s and above.";


NOT_SUPPORTED_RUNTIME_VERSION?

Fatme · 2018-11-01T15:10:03Z

lib/helpers/android-bundle-validator-helper.ts

+			const androidRuntimeVersion = androidRuntimeInfo ? androidRuntimeInfo.version : "";
+
+			if (androidRuntimeVersion) {
+				const shouldSkipCheck = !semver.valid(androidRuntimeVersion) && !semver.validRange(androidRuntimeVersion);


This code is duplicated with the code in bundle-validator-helper.ts. So I suggest to extract a helper method

public isVersionSupported(version: string, minSupportedVersion: string): boolean { const shouldSkipCheck = !semver.valid(version) && !semver.validRange(version); if(shouldSkipCheck) { return true; } const isVersionSupported = semver.gte(semver.coerce(version), semver.coerce(minSupportedVersion)); return isVersionSupported; }

const isAndroidBundleSupported = helpers.isVersionSupported(androidRuntimeVersion, AndroidBundleValidatorHelper.MIN_RUNTIME_VERSION); if (!isAndroidBundleSupported) { this.$errors.failWithoutHelp(..); }

I have considered a similar approach, but it seemed strange to extract a method with such specific use case. By specific use case I mean - behavior if the version is not valid(someone might want to error in this case) and gte (this might be adjusted by the required version, but still requires knowledge about isVersionSupported inner working).

If you insist I will extract it in separate class and extend the others, but I don't think anyone will ever use it elsewhere.

Might extract !semver.valid(androidRuntimeVersion) && !semver.validRange(androidRuntimeVersion); as a helper method.

DimitarTachev · 2018-11-02T09:39:51Z

lib/commands/build.ts

+		const buildResult = await this.executeCore([this.$platformsData.availablePlatforms.Android]);
+
+		if (this.$options.aab) {
+			this.$logger.info(`Your .aab file is located at: ${buildResult}`);


We could also print the apk output

DimitarTachev · 2018-11-02T09:45:52Z

lib/declarations.d.ts

-interface IAndroidBuildOptionsSettings extends IAndroidReleaseOptions, IRelease { }
+interface IAndroidBuildOptionsSettings extends IAndroidReleaseOptions, IRelease, IAndroidBundle { }
+
+ interface IAndroidBundle {


IAndroidBundle -> IHasAndroidBundle

gezn · 2018-11-06T20:39:56Z

when this will be merged ? can't wait to test it and see the app sizes for different porpouses. hope this improve size a lot. Thanks @KristianDD

gezn · 2018-11-06T20:40:59Z

when this will be merged ? can't wait to test it and see the app sizes for different porpouses. hope this improve size a lot. Thanks @KristianDD

Fatme · 2018-11-06T21:59:19Z

lib/helpers/android-bundle-validator-helper.ts

-						this.$errors.failWithoutHelp(util.format(AndroidBundleValidatorMessages.RUNTIME_VERSION_TOO_LOW, AndroidBundleValidatorHelper.MIN_RUNTIME_VERSION));
-					}
-				}
+			if (androidRuntimeVersion &&


Cool! I really like it. But we can make it even better.

const shouldThrowError = androidRuntimeVersion && this.isValidVersion(androidRuntimeVersion) && this.isVersionLowerThan(androidRuntimeVersion, AndroidBundleValidatorHelper.MIN_RUNTIME_VERSION); if (shouldThrowError) { ... }

So no need to provide semver.gte function as a param to another function.

isVersionLowerThan is actually misleading, because it does not imply for gte comparison, but gt comparison. I prefer sending a function to be used to compare the values. This makes the helper much more versatile for future use.

"divide and conquer" approach

isVersionLowerThan(..) isVersionLowerOrEqualThan(..) isVersionGreaterThan(...) isVersionGreaterOrEqualThan(...)

If we decide to change the implementation or add additional logic to some of the above functions, the change will affect ONLY the specific function. So we will have simplification + more control over the affected parts.

compareCoerceVersions(androidRuntimeVersion, AndroidBundleValidatorHelper.MIN_RUNTIME_VERSION, semver.gte) compareCoerceVersions(androidRuntimeVersion, AndroidBundleValidatorHelper.MIN_RUNTIME_VERSION, semver.gt) compareCoerceVersions(androidRuntimeVersion, AndroidBundleValidatorHelper.MIN_RUNTIME_VERSION, semver.lte) compareCoerceVersions(androidRuntimeVersion, AndroidBundleValidatorHelper.MIN_RUNTIME_VERSION, semver.lt)

If we decide to change the implementation of compareCoerceVersions it will affect ALL the calls above.
So we will wonder "how this will affect the case when the function is called for example with semver.gt" or Will it affect the case when passed function is semver.lte. So it will be harder to read + less control over the affected parts.

Even if I create a wrapper for each comparison semver has, it will underneath use the same private method that wraps the same logic (getting the coerce of each version and invoking a function). So changing the private method will affect the same amount of places, but indirectly.

@rosen-vladimirov @DimitarTachev I would appreciate your input on the matter. If the majority considers it's better to wrap semver methods I will do it.

I prefer a few but simple functions. Methods like isVersionLowerThan are more clear, simple and easy to be unit tested. It's much easier to do one thing and do it well for such functions. In other words, I prefer sticking to simple functions till we don't need more complex scenario requiring some custom comparisons.

ggarabedian · 2018-11-07T12:56:08Z

lib/commands/build.ts

 		if (this.$options.copyTo) {
 			this.$platformService.copyLastOutput(platform, this.$options.copyTo, buildConfig, this.$projectData);
+		} else {
+			this.$logger.info(`Your build result is located at: ${outputPath}`);


I am not very familiar with the CLI errors, but if you/your is not used a lot, I would recommend changing this to "The build result is located at" instead of "Your..."

ggarabedian · 2018-11-07T12:58:00Z

lib/services/platform-service.ts

-	public async validateOptions(provision: true | string, teamId: true | string, projectData: IProjectData, platform?: string): Promise<boolean> {
+	public async validateOptions(provision: true | string, teamId: true | string, projectData: IProjectData, platform?: string, aab?: boolean): Promise<boolean> {
+		if (platform && !this.$mobileHelper.isAndroidPlatform(platform) && aab) {
+			this.$errors.failWithoutHelp("Option --aab is supported for Android platform only.");


I would recommend rephrasing this to "The --aab option is supported only for the Android platform" or "The --aab option is supported only for Android builds".

ggarabedian · 2018-11-07T13:03:24Z

docs/man_pages/project/testing/build-android.md

@@ -27,6 +27,7 @@ General | `$ tns build android [--compileSdk <API Level>] [--key-store-path <Fil
 * `--copy-to` - Specifies the file path where the built `.apk` will be copied. If it points to a non-existent directory, it will be created. If the specified value is directory, the original file name will be used.
 * `--bundle` - Specifies that the `webpack` bundler will be used to bundle the application.
 * `--env.*` - Specifies additional flags that the bundler may process. May be passed multiple times. For example: `--env.uglify --env.snapshot`.
+* `--aab` - Specifies that the build must produce an Android App Bundle(`.aab`) file.


You should consider replacing "must" with "will". It sounds better to say what the command will do :)

ggarabedian

👍 for the texts

KristianDD force-pushed the kddimitrov/aab-build branch from 8f3cc2a to cdb1ae1 Compare November 1, 2018 14:24

KristianDD added 8 commits November 2, 2018 11:16

feat: inital work on Android Application Bundle Build (.aab)

1860059

chore: unify interfaces

58d57fd

feat: add validations for --aab command

dac46b7

fix: hashes with --aab outputed in wrong directory

61d9843

feat: validation for runtime version for --aab, linting, tests fix

4bb3bdc

test: add tests for androidBundle

d40fc5a

feat: print output path of .aab file

2cb7013

chore: lint and remove only in tests

d5432f7

KristianDD force-pushed the kddimitrov/aab-build branch from cdb1ae1 to d5432f7 Compare November 2, 2018 09:20

Fatme reviewed Nov 2, 2018

View reviewed changes

DimitarTachev approved these changes Nov 2, 2018

View reviewed changes

NativeScript deleted a comment from DimitarTachev Nov 2, 2018

chore: fix comments

ba7cac4

Fatme reviewed Nov 6, 2018

View reviewed changes

chore: add help for aab flag

11bd5ba

ggarabedian reviewed Nov 7, 2018

View reviewed changes

chore: fix aab messages after review

a29ced3

ggarabedian approved these changes Nov 7, 2018

View reviewed changes

chore: fix comments aab

a6dcfb9

KristianDD force-pushed the kddimitrov/aab-build branch from df65d21 to a6dcfb9 Compare November 8, 2018 08:25

KristianDD merged commit cd2aad3 into master Nov 8, 2018

KristianDD deleted the kddimitrov/aab-build branch November 8, 2018 11:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kddimitrov/aab build #4084

Kddimitrov/aab build #4084

KristianDD commented Nov 1, 2018

Fatme Nov 1, 2018

KristianDD Nov 2, 2018 •

edited

Loading

KristianDD Nov 2, 2018 •

edited

Loading

Fatme Nov 1, 2018

KristianDD Nov 2, 2018

Fatme Nov 1, 2018

Fatme Nov 1, 2018

Fatme Nov 1, 2018

KristianDD Nov 2, 2018

Fatme Nov 1, 2018

Fatme Nov 1, 2018

KristianDD Nov 2, 2018

KristianDD Nov 2, 2018

DimitarTachev Nov 2, 2018 •

edited

Loading

DimitarTachev Nov 2, 2018

gezn commented Nov 6, 2018

gezn commented Nov 6, 2018

Fatme Nov 6, 2018

KristianDD Nov 7, 2018

Fatme Nov 7, 2018

KristianDD Nov 7, 2018

KristianDD Nov 7, 2018 •

edited

Loading

DimitarTachev Nov 7, 2018

ggarabedian Nov 7, 2018

ggarabedian Nov 7, 2018

ggarabedian Nov 7, 2018

ggarabedian left a comment

Kddimitrov/aab build #4084

Kddimitrov/aab build #4084

Conversation

KristianDD commented Nov 1, 2018

PR Checklist

What is the current behavior?

What is the new behavior?

Choose a reason for hiding this comment

KristianDD Nov 2, 2018 • edited Loading

Choose a reason for hiding this comment

KristianDD Nov 2, 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

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 Nov 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gezn commented Nov 6, 2018

gezn commented Nov 6, 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

KristianDD Nov 7, 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

ggarabedian left a comment

Choose a reason for hiding this comment

KristianDD Nov 2, 2018 •

edited

Loading

KristianDD Nov 2, 2018 •

edited

Loading

DimitarTachev Nov 2, 2018 •

edited

Loading

KristianDD Nov 7, 2018 •

edited

Loading