Skip to content

Kddimitrov/aab build #4084

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Nov 8, 2018
Merged

Kddimitrov/aab build #4084

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1860059
feat: inital work on Android Application Bundle Build (.aab)
KristianDD Oct 25, 2018
58d57fd
chore: unify interfaces
KristianDD Oct 25, 2018
dac46b7
feat: add validations for --aab command
KristianDD Oct 26, 2018
61d9843
fix: hashes with --aab outputed in wrong directory
KristianDD Oct 30, 2018
4bb3bdc
feat: validation for runtime version for --aab, linting, tests fix
KristianDD Oct 31, 2018
d40fc5a
test: add tests for androidBundle
KristianDD Oct 31, 2018
2cb7013
feat: print output path of .aab file
KristianDD Nov 1, 2018
d5432f7
chore: lint and remove only in tests
KristianDD Nov 1, 2018
ba7cac4
chore: fix comments
KristianDD Nov 6, 2018
11bd5ba
chore: add help for aab flag
KristianDD Nov 7, 2018
a29ced3
chore: fix aab messages after review
KristianDD Nov 7, 2018
a6dcfb9
chore: fix comments aab
KristianDD Nov 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion docs/man_pages/project/testing/build-android.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ Builds the project for Android and produces an APK that you can manually deploy

Usage | Synopsis
---|---
General | `$ tns build android [--compileSdk <API Level>] [--key-store-path <File Path> --key-store-password <Password> --key-store-alias <Name> --key-store-alias-password <Password>] [--release] [--static-bindings] [--copy-to <File Path>] [--bundle [<value>] [--env.*]]`
General | `$ tns build android [--compileSdk <API Level>] [--key-store-path <File Path> --key-store-password <Password> --key-store-alias <Name> --key-store-alias-password <Password>] [--release] [--static-bindings] [--copy-to <File Path>] [--bundle [<value>] [--env.*]] [--aab]`

### Options

Expand All @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


<% if(isHtml) { %>

Expand Down
1 change: 1 addition & 0 deletions lib/bootstrap.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ $injector.require("devicePathProvider", "./device-path-provider");
$injector.requireCommand("platform|clean", "./commands/platform-clean");

$injector.require("bundleValidatorHelper", "./helpers/bundle-validator-helper");
$injector.require("androidBundleValidatorHelper", "./helpers/android-bundle-validator-helper");
$injector.require("liveSyncCommandHelper", "./helpers/livesync-command-helper");
$injector.require("deployCommandHelper", "./helpers/deploy-command-helper");

Expand Down
43 changes: 31 additions & 12 deletions lib/commands/build.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { ANDROID_RELEASE_BUILD_ERROR_MESSAGE } from "../constants";
import { ANDROID_RELEASE_BUILD_ERROR_MESSAGE, AndroidAppBundleMessages } from "../constants";
import { ValidatePlatformCommandBase } from "./command-base";

export abstract class BuildCommandBase extends ValidatePlatformCommandBase {
Expand All @@ -8,12 +8,13 @@ export abstract class BuildCommandBase extends ValidatePlatformCommandBase {
$platformsData: IPlatformsData,
protected $devicePlatformsConstants: Mobile.IDevicePlatformsConstants,
$platformService: IPlatformService,
private $bundleValidatorHelper: IBundleValidatorHelper) {
private $bundleValidatorHelper: IBundleValidatorHelper,
protected $logger: ILogger) {
super($options, $platformsData, $platformService, $projectData);
this.$projectData.initializeProjectData();
}

public async executeCore(args: string[]): Promise<void> {
public async executeCore(args: string[]): Promise<string> {
const platform = args[0].toLowerCase();
const appFilesUpdaterOptions: IAppFilesUpdaterOptions = {
bundle: !!this.$options.bundle,
Expand Down Expand Up @@ -41,12 +42,19 @@ export abstract class BuildCommandBase extends ValidatePlatformCommandBase {
keyStoreAlias: this.$options.keyStoreAlias,
keyStorePath: this.$options.keyStorePath,
keyStoreAliasPassword: this.$options.keyStoreAliasPassword,
keyStorePassword: this.$options.keyStorePassword
keyStorePassword: this.$options.keyStorePassword,
androidBundle: this.$options.aab
};
await this.$platformService.buildPlatform(platform, buildConfig, this.$projectData);

const outputPath = await this.$platformService.buildPlatform(platform, buildConfig, this.$projectData);

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}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..."

}

return outputPath;
}

protected validatePlatform(platform: string): void {
Expand Down Expand Up @@ -84,12 +92,13 @@ export class BuildIosCommand extends BuildCommandBase implements ICommand {
$platformsData: IPlatformsData,
$devicePlatformsConstants: Mobile.IDevicePlatformsConstants,
$platformService: IPlatformService,
$bundleValidatorHelper: IBundleValidatorHelper) {
super($options, $errors, $projectData, $platformsData, $devicePlatformsConstants, $platformService, $bundleValidatorHelper);
$bundleValidatorHelper: IBundleValidatorHelper,
$logger: ILogger) {
super($options, $errors, $projectData, $platformsData, $devicePlatformsConstants, $platformService, $bundleValidatorHelper, $logger);
}

public async execute(args: string[]): Promise<void> {
return this.executeCore([this.$platformsData.availablePlatforms.iOS]);
await this.executeCore([this.$platformsData.availablePlatforms.iOS]);
}

public async canExecute(args: string[]): Promise<boolean | ICanExecuteCommandOutput> {
Expand Down Expand Up @@ -117,18 +126,28 @@ export class BuildAndroidCommand extends BuildCommandBase implements ICommand {
$platformsData: IPlatformsData,
$devicePlatformsConstants: Mobile.IDevicePlatformsConstants,
$platformService: IPlatformService,
$bundleValidatorHelper: IBundleValidatorHelper) {
super($options, $errors, $projectData, $platformsData, $devicePlatformsConstants, $platformService, $bundleValidatorHelper);
$bundleValidatorHelper: IBundleValidatorHelper,
protected $androidBundleValidatorHelper: IAndroidBundleValidatorHelper,
protected $logger: ILogger) {
super($options, $errors, $projectData, $platformsData, $devicePlatformsConstants, $platformService, $bundleValidatorHelper, $logger);
}

public async execute(args: string[]): Promise<void> {
return this.executeCore([this.$platformsData.availablePlatforms.Android]);
await this.executeCore([this.$platformsData.availablePlatforms.Android]);

if (this.$options.aab) {
this.$logger.info(AndroidAppBundleMessages.ANDROID_APP_BUNDLE_DOCS_MESSAGE);

if (this.$options.release) {
this.$logger.info(AndroidAppBundleMessages.ANDROID_APP_BUNDLE_PUBLISH_DOCS_MESSAGE);
}
}
}

public async canExecute(args: string[]): Promise<boolean | ICanExecuteCommandOutput> {
const platform = this.$devicePlatformsConstants.Android;
super.validatePlatform(platform);

this.$androidBundleValidatorHelper.validateRuntimeVersion(this.$projectData);
let result = await super.canExecuteCommandBase(platform, { notConfiguredEnvOptions: { hideSyncToPreviewAppOption: true }});
if (result.canExecute) {
if (this.$options.release && (!this.$options.keyStorePath || !this.$options.keyStorePassword || !this.$options.keyStoreAlias || !this.$options.keyStoreAliasPassword)) {
Expand Down
5 changes: 4 additions & 1 deletion lib/commands/debug.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ export class DebugPlatformCommand extends ValidatePlatformCommandBase implements
private $debugDataService: IDebugDataService,
private $liveSyncService: IDebugLiveSyncService,
private $prompter: IPrompter,
private $liveSyncCommandHelper: ILiveSyncCommandHelper) {
private $liveSyncCommandHelper: ILiveSyncCommandHelper,
private $androidBundleValidatorHelper: IAndroidBundleValidatorHelper) {
super($options, $platformsData, $platformService, $projectData);
}

Expand Down Expand Up @@ -108,6 +109,8 @@ export class DebugPlatformCommand extends ValidatePlatformCommandBase implements
}

public async canExecute(args: string[]): Promise<ICanExecuteCommandOutput> {
this.$androidBundleValidatorHelper.validateNoAab();

if (!this.$platformService.isPlatformSupportedForOS(this.platform, this.$projectData)) {
this.$errors.fail(`Applications for platform ${this.platform} can not be built on this OS`);
}
Expand Down
4 changes: 3 additions & 1 deletion lib/commands/deploy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ export class DeployOnDeviceCommand extends ValidatePlatformCommandBase implement
private $errors: IErrors,
private $mobileHelper: Mobile.IMobileHelper,
$platformsData: IPlatformsData,
private $bundleValidatorHelper: IBundleValidatorHelper) {
private $bundleValidatorHelper: IBundleValidatorHelper,
private $androidBundleValidatorHelper: IAndroidBundleValidatorHelper) {
super($options, $platformsData, $platformService, $projectData);
this.$projectData.initializeProjectData();
}
Expand All @@ -24,6 +25,7 @@ export class DeployOnDeviceCommand extends ValidatePlatformCommandBase implement
}

public async canExecute(args: string[]): Promise<boolean | ICanExecuteCommandOutput> {
this.$androidBundleValidatorHelper.validateNoAab();
this.$bundleValidatorHelper.validate();
if (!args || !args.length || args.length > 1) {
return false;
Expand Down
2 changes: 1 addition & 1 deletion lib/commands/plugin/build-plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export class BuildPluginCommand implements ICommand {
temp.track();
const tempAndroidProject = temp.mkdirSync("android-project");

const options: IBuildOptions = {
const options: IPluginBuildOptions = {
aarOutputDir: platformsAndroidPath,
platformsAndroidDirPath: platformsAndroidPath,
pluginName: pluginName,
Expand Down
14 changes: 8 additions & 6 deletions lib/commands/run.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ export class RunCommandBase implements ICommand {
private $devicePlatformsConstants: Mobile.IDevicePlatformsConstants,
private $errors: IErrors,
private $hostInfo: IHostInfo,
private $liveSyncCommandHelper: ILiveSyncCommandHelper) { }
private $liveSyncCommandHelper: ILiveSyncCommandHelper,
private $androidBundleValidatorHelper: IAndroidBundleValidatorHelper) { }

public allowedParameters: ICommandParameter[] = [];
public async execute(args: string[]): Promise<void> {
Expand All @@ -22,6 +23,8 @@ export class RunCommandBase implements ICommand {
this.$errors.fail(ERROR_NO_VALID_SUBCOMMAND_FORMAT, "run");
}

this.$androidBundleValidatorHelper.validateNoAab();

this.$projectData.initializeProjectData();
this.platform = args[0] || this.platform;

Expand All @@ -35,7 +38,6 @@ export class RunCommandBase implements ICommand {
const checkEnvironmentRequirementsOutput = validatePlatformOutput[this.platform.toLowerCase()].checkEnvironmentRequirementsOutput;
this.liveSyncCommandHelperAdditionalOptions.syncToPreviewApp = checkEnvironmentRequirementsOutput && checkEnvironmentRequirementsOutput.selectedOption === "Sync to Playground";
}

return true;
}
}
Expand Down Expand Up @@ -66,14 +68,14 @@ export class RunIosCommand implements ICommand {
}

public async execute(args: string[]): Promise<void> {
if (!this.$platformService.isPlatformSupportedForOS(this.$devicePlatformsConstants.iOS, this.$projectData)) {
this.$errors.fail(`Applications for platform ${this.$devicePlatformsConstants.iOS} can not be built on this OS`);
}

return this.runCommand.execute(args);
}

public async canExecute(args: string[]): Promise<boolean> {
if (!this.$platformService.isPlatformSupportedForOS(this.$devicePlatformsConstants.iOS, this.$projectData)) {
this.$errors.fail(`Applications for platform ${this.$devicePlatformsConstants.iOS} can not be built on this OS`);
}

const result = await this.runCommand.canExecute(args) && await this.$platformService.validateOptions(this.$options.provision, this.$options.teamId, this.$projectData, this.$platformsData.availablePlatforms.iOS);
return result;
}
Expand Down
12 changes: 12 additions & 0 deletions lib/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,14 @@ export const BUILD_XCCONFIG_FILE_NAME = "build.xcconfig";
export const BUILD_DIR = "build";
export const OUTPUTS_DIR = "outputs";
export const APK_DIR = "apk";
export const BUNDLE_DIR = "bundle";
export const RESOURCES_DIR = "res";
export const CONFIG_NS_FILE_NAME = "nsconfig.json";
export const CONFIG_NS_APP_RESOURCES_ENTRY = "appResourcesPath";
export const CONFIG_NS_APP_ENTRY = "appPath";
export const DEPENDENCIES_JSON_NAME = "dependencies.json";
export const APK_EXTENSION_NAME = ".apk";
export const AAB_EXTENSION_NAME = ".aab";
export const HASHES_FILE_NAME = ".nshashes";

export class PackageVersion {
Expand Down Expand Up @@ -241,3 +243,13 @@ 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 App Bundle) parameter.";
public static NOT_SUPPORTED_RUNTIME_VERSION = "Android App Bundle (--aab) option requires NativeScript Android Runtime (tns-android) version %s and above.";
}

export class AndroidAppBundleMessages {
public static ANDROID_APP_BUNDLE_DOCS_MESSAGE = "What is Android App Bundle: https://docs.nativescript.org/tooling/publishing/android-app-bundle";
public static ANDROID_APP_BUNDLE_PUBLISH_DOCS_MESSAGE = "How to use Android App Bundle for publishing: https://docs.nativescript.org/tooling/publishing/publishing-android-apps#android-app-bundle";
}
29 changes: 27 additions & 2 deletions lib/declarations.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,7 +458,11 @@ interface IPluginSeedOptions {
pluginName: string;
}

interface IOptions extends IRelease, IDeviceIdentifier, IJustLaunch, IAvd, IAvailableDevices, IProfileDir, IHasEmulatorOption, IBundleString, IPlatformTemplate, IHasEmulatorOption, IClean, IProvision, ITeamIdentifier, IAndroidReleaseOptions, INpmInstallConfigurationOptions, IPort, IEnvOptions, IPluginSeedOptions, IGenerateOptions {
interface IAndroidBundleOptions {
aab: boolean;
}

interface IOptions extends IRelease, IDeviceIdentifier, IJustLaunch, IAvd, IAvailableDevices, IProfileDir, IHasEmulatorOption, IBundleString, IPlatformTemplate, IHasEmulatorOption, IClean, IProvision, ITeamIdentifier, IAndroidReleaseOptions, IAndroidBundleOptions, INpmInstallConfigurationOptions, IPort, IEnvOptions, IPluginSeedOptions, IGenerateOptions {
argv: IYargArgv;
validateOptions(commandSpecificDashedOptions?: IDictionary<IDashedOption>): void;
options: IDictionary<IDashedOption>;
Expand Down Expand Up @@ -531,7 +535,11 @@ interface IEnvOptions {
env: Object;
}

interface IAndroidBuildOptionsSettings extends IAndroidReleaseOptions, IRelease { }
interface IAndroidBuildOptionsSettings extends IAndroidReleaseOptions, IRelease, IHasAndroidBundle { }

interface IHasAndroidBundle {
androidBundle?: boolean;
}

interface IAppFilesUpdaterOptions extends IBundle, IRelease, IOptionalWatchAllFiles, IHasUseHotModuleReloadOption { }

Expand Down Expand Up @@ -856,6 +864,23 @@ interface IBundleValidatorHelper {
validate(minSupportedVersion?: string): void;
}


interface IAndroidBundleValidatorHelper {
/**
* Validates android bundling option is not provided.
* Commands that require deploy of the application must not be called with --aab option
* @return {void}
*/
validateNoAab(): void;

/**
* Validates android runtime version is sufficient to support bundling option --aab.
* @param {IProjectData} projectData DTO with information about the project.
* @return {void}
*/
validateRuntimeVersion(projectData: IProjectData): void
}

interface INativeScriptCloudExtensionService {
/**
* Installs nativescript-cloud extension
Expand Down
6 changes: 3 additions & 3 deletions lib/definitions/android-plugin-migrator.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@

interface IBuildOptions extends IAndroidBuildOptions{
interface IPluginBuildOptions extends IAndroidBuildOptions{
projectDir?: string
}

Expand All @@ -11,8 +11,8 @@ interface IAndroidBuildOptions {
}

interface IAndroidPluginBuildService {
buildAar(options: IBuildOptions): Promise<boolean>;
migrateIncludeGradle(options: IBuildOptions): boolean;
buildAar(options: IPluginBuildOptions): Promise<boolean>;
migrateIncludeGradle(options: IPluginBuildOptions): boolean;
}

/**
Expand Down
6 changes: 2 additions & 4 deletions lib/definitions/platform.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,7 @@ interface IPlatformData {
normalizedPlatformName: string;
appDestinationDirectoryPath: string;
deviceBuildOutputPath: string;
bundleBuildOutputPath?: string;
emulatorBuildOutputPath?: string;
getValidBuildOutputData(buildOptions: IBuildOutputOptions): IValidBuildOutputData;
frameworkFilesExtensions: string[];
Expand All @@ -285,10 +286,7 @@ interface IValidBuildOutputData {
regexes?: RegExp[];
}

interface IBuildOutputOptions {
isReleaseBuild?: boolean;
isForDevice?: boolean;
}
interface IBuildOutputOptions extends Partial<IBuildForDevice>, IRelease, IHasAndroidBundle {}

interface IPlatformsData {
availablePlatforms: any;
Expand Down
2 changes: 1 addition & 1 deletion lib/definitions/project.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -437,7 +437,7 @@ interface IPlatformProjectService extends NodeJS.EventEmitter, IPlatformProjectS
/**
* Build native part of a nativescript plugins if necessary
*/
prebuildNativePlugin(buildOptions: IBuildOptions): Promise<void>;
prebuildNativePlugin(buildOptions: IPluginBuildOptions): Promise<void>;

/**
* Traverse through the production dependencies and find plugins that need build/rebuild
Expand Down
36 changes: 36 additions & 0 deletions lib/helpers/android-bundle-validator-helper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import * as semver from "semver";
import * as util from "util";
import { AndroidBundleValidatorMessages, TNS_ANDROID_RUNTIME_NAME } from "../constants";
import { VersionValidatorHelper } from "./version-validator-helper";

export class AndroidBundleValidatorHelper extends VersionValidatorHelper implements IAndroidBundleValidatorHelper {
public static MIN_RUNTIME_VERSION = "5.0.0";

constructor(protected $projectData: IProjectData,
protected $errors: IErrors,
protected $options: IOptions,
protected $projectDataService: IProjectDataService) {
super();
}

public validateNoAab(): void {
if (this.$options.aab) {
this.$errors.fail(AndroidBundleValidatorMessages.AAB_NOT_SUPPORTED_BY_COMMNAND_MESSAGE);
}
}

public validateRuntimeVersion(projectData: IProjectData): void {
if (this.$options.aab) {
const androidRuntimeInfo = this.$projectDataService.getNSValue(projectData.projectDir, TNS_ANDROID_RUNTIME_NAME);
const androidRuntimeVersion = androidRuntimeInfo ? androidRuntimeInfo.version : "";

if (androidRuntimeVersion &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. "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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@KristianDD KristianDD Nov 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

this.isValidVersion(androidRuntimeVersion) &&
!this.compareCoerceVersions(androidRuntimeVersion, AndroidBundleValidatorHelper.MIN_RUNTIME_VERSION, semver.gte)) {
this.$errors.failWithoutHelp(util.format(AndroidBundleValidatorMessages.NOT_SUPPORTED_RUNTIME_VERSION, AndroidBundleValidatorHelper.MIN_RUNTIME_VERSION));
}
}
}
}

$injector.register("androidBundleValidatorHelper", AndroidBundleValidatorHelper);
Loading