From 80e6ca819b37bbb2170ae77d4895775d4b01d625 Mon Sep 17 00:00:00 2001 From: rosen-vladimirov Date: Fri, 31 May 2019 17:43:01 +0300 Subject: [PATCH] fix: cloud run command does not respect useLegacyWorkflow flag Currently when you run `tns cloud run ...` the command executes local prepare without taking into account the value of `useLegacyWorkflow` value in nsconfig.json file. The problem is that cloud commands have specific `--` options, which are set as `dashedOptions` on command level. During command execution the following actions happen: 1. CLI starts its execution and via `$injector` constructs new instance of `$options`. 2. In the constructor of `$options` we call `this.setArgv()`, which calls `yargs` package and constructs values based on the user input of `--` options. 3. `$injector` resolves `$commandsService` and constructs new object from it. 4. In the constructor of `CommandsService`, we call `$options.setupOptions` and pass the current projectData. `setupOptions` takes into account the projectData and again calls `this.setArgv()`. After that it overwrites some of the values set by yargs based on the projectData. 5. `CommandsService` starts execution of the command by resolving new instance of it and calling its own `tryExecuteCommandAction` method. 6. `tryExecuteCommandAction` method internally calls `this.$options.validateOptions` by passing the command specific options(`dashedOptions`). The `validateOptions` method modifies the internal structure based on which yargs constructs the passed options and calls `this.setArgv()` again. At this point we overwrite the internal values of `$options` based on the user's input and taking into account the command's `dashedOptions`. However, this way we've overwritten the values set by `setupOptions` in step 4 which are based on the current project dir. To fix the behavior, make `setupOptions` private in `$options` and call it internally in the `validateOptions` method. So the new workflow is: 1. CLI starts its execution and via `$injector` constructs new instance of `$options`. 2. In the constructor of `$options` we call `this.setArgv()`, which calls `yargs` package and constructs values based on the user input of `--` options. 3. `$injector` resolves `$commandsService` and constructs new object from it. 4. `CommandsService` starts execution of the command by resolving new instance of it and calling its own `tryExecuteCommandAction` method. 5. `tryExecuteCommandAction` method internally calls `this.$options.validateOptions` by passing the command specific options(`dashedOptions`). The `validateOptions` method calls `this.setupOptions` and pass the current projectData and command specific options. After that it calls `this.setArgv()` and the internal structure that contains the passed options is overwritten based on the command specific options. After that the method overwrites some of the values based on the passed projectData. --- lib/common/services/commands-service.ts | 22 +++++++++++----------- lib/declarations.d.ts | 3 +-- lib/options.ts | 15 ++++++++------- test/options.ts | 4 ++-- 4 files changed, 22 insertions(+), 22 deletions(-) diff --git a/lib/common/services/commands-service.ts b/lib/common/services/commands-service.ts index 315bbfa6e1..f46e63e4b1 100644 --- a/lib/common/services/commands-service.ts +++ b/lib/common/services/commands-service.ts @@ -29,15 +29,6 @@ export class CommandsService implements ICommandsService { private $extensibilityService: IExtensibilityService, private $optionsTracker: IOptionsTracker, private $projectDataService: IProjectDataService) { - let projectData = null; - try { - projectData = this.$projectDataService.getProjectData(); - } catch (err) { - this.$logger.trace(`Error while trying to get project data. More info: ${err}`); - } - - this.$options.setupOptions(projectData); - this.$options.printMessagesForDeprecatedOptions(this.$logger); } public allCommands(opts: { includeDevCommands: boolean }): string[] { @@ -114,8 +105,17 @@ export class CommandsService implements ICommandsService { private async tryExecuteCommandAction(commandName: string, commandArguments: string[]): Promise { const command = this.$injector.resolveCommand(commandName); - if (!command || (command && !command.isHierarchicalCommand)) { - this.$options.validateOptions(command ? command.dashedOptions : null); + if (!command || !command.isHierarchicalCommand) { + let projectData = null; + try { + projectData = this.$projectDataService.getProjectData(); + } catch (err) { + this.$logger.trace(`Error while trying to get project data. More info: ${err}`); + } + + const dashedOptions = command ? command.dashedOptions : null; + this.$options.validateOptions(dashedOptions, projectData); + this.$options.printMessagesForDeprecatedOptions(this.$logger); } return this.canExecuteCommand(commandName, commandArguments); diff --git a/lib/declarations.d.ts b/lib/declarations.d.ts index 24e53f6c37..9a96673a2e 100644 --- a/lib/declarations.d.ts +++ b/lib/declarations.d.ts @@ -503,7 +503,7 @@ interface IAndroidBundleOptions { 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): void; + validateOptions(commandSpecificDashedOptions?: IDictionary, projectData?: IProjectData): void; options: IDictionary; shorthands: string[]; /** @@ -573,7 +573,6 @@ interface IOptions extends IRelease, IDeviceIdentifier, IJustLaunch, IAvd, IAvai performance: Object; cleanupLogFile: string; workflow: any; - setupOptions(projectData: IProjectData): void; printMessagesForDeprecatedOptions(logger: ILogger): void; } diff --git a/lib/options.ts b/lib/options.ts index 01936ee917..8083b69425 100644 --- a/lib/options.ts +++ b/lib/options.ts @@ -21,7 +21,12 @@ export class Options { public options: IDictionary; - public setupOptions(projectData: IProjectData): void { + public setupOptions(projectData: IProjectData, commandSpecificDashedOptions?: IDictionary): void { + if (commandSpecificDashedOptions) { + _.extend(this.options, commandSpecificDashedOptions); + this.setArgv(); + } + if (this.argv.release && this.argv.hmr) { this.$errors.failWithoutHelp("The options --release and --hmr cannot be used simultaneously."); } @@ -173,12 +178,8 @@ export class Options { return this.argv[optionName]; } - public validateOptions(commandSpecificDashedOptions?: IDictionary): void { - if (commandSpecificDashedOptions) { - _.extend(this.options, commandSpecificDashedOptions); - this.setArgv(); - } - + public validateOptions(commandSpecificDashedOptions?: IDictionary, projectData?: IProjectData): void { + this.setupOptions(projectData, commandSpecificDashedOptions); const parsed = Object.create(null); // DO NOT REMOVE { } as when they are missing and some of the option values is false, the each stops as it thinks we have set "return false". _.each(_.keys(this.argv), optionName => { diff --git a/test/options.ts b/test/options.ts index 1727991746..444fbf444b 100644 --- a/test/options.ts +++ b/test/options.ts @@ -325,7 +325,7 @@ describe("options", () => { it(`should pass correctly when ${testCase.name} and useLegacyWorkflow is ${useLegacyWorkflow}`, () => { (testCase.args || []).forEach(arg => process.argv.push(arg)); - const options = createOptions(testInjector); + const options: any = createOptions(testInjector); const projectData = { useLegacyWorkflow }; options.setupOptions(projectData); @@ -359,7 +359,7 @@ describe("options", () => { errors.failWithoutHelp = (error: string) => actualError = error; (testCase.args || []).forEach(arg => process.argv.push(arg)); - const options = createOptions(testInjector); + const options: any = createOptions(testInjector); options.setupOptions(null); (testCase.args || []).forEach(arg => process.argv.pop());