From a7b754a88d02d7660195081ad9f1802dcd5a25d6 Mon Sep 17 00:00:00 2001 From: Rosen Vladimirov Date: Sat, 26 Jan 2019 12:52:28 +0200 Subject: [PATCH 1/2] feat: skip postinstall steps in case CLI is not installed globally In case the CLI package is not installed globally, probably it is installed as a dependency of a project or as a dependency of other dependency. In this case the postinstall actions have no meaning for the user, as probably CLI will be used as a library. Skip all of the postinstall actions in such case. Remove the execution code of the old `post-install` command and move it to `post-install-cli` command. Add unit tests. Remove a property from staticConfig - it has no meaning to be there, just place it in the postinstall command directly. --- lib/commands/post-install.ts | 42 ++++++--- lib/common/commands/post-install.ts | 30 +----- lib/common/commands/preuninstall.ts | 22 +---- lib/common/definitions/config.d.ts | 1 - lib/common/helpers.ts | 56 ++++++++++++ lib/common/test/unit-tests/helpers.ts | 126 ++++++++++++++++++++++++++ lib/config.ts | 1 - postinstall.js | 6 +- test/commands/post-install.ts | 2 + test/post-install.ts | 30 ++++-- 10 files changed, 245 insertions(+), 71 deletions(-) diff --git a/lib/commands/post-install.ts b/lib/commands/post-install.ts index 212ad64155..d3f998e157 100644 --- a/lib/commands/post-install.ts +++ b/lib/commands/post-install.ts @@ -1,21 +1,35 @@ -import { PostInstallCommand } from "../common/commands/post-install"; - -export class PostInstallCliCommand extends PostInstallCommand { - constructor($fs: IFileSystem, +export class PostInstallCliCommand implements ICommand { + constructor(private $fs: IFileSystem, private $subscriptionService: ISubscriptionService, - $staticConfig: Config.IStaticConfig, - $commandsService: ICommandsService, - $helpService: IHelpService, - $settingsService: ISettingsService, - $doctorService: IDoctorService, - $analyticsService: IAnalyticsService, - $logger: ILogger) { - super($fs, $staticConfig, $commandsService, $helpService, $settingsService, $analyticsService, $logger); + private $commandsService: ICommandsService, + private $helpService: IHelpService, + private $settingsService: ISettingsService, + private $analyticsService: IAnalyticsService, + private $logger: ILogger, + private $hostInfo: IHostInfo) { } - public async execute(args: string[]): Promise { - await super.execute(args); + public disableAnalytics = true; + public allowedParameters: ICommandParameter[] = []; + public async execute(args: string[]): Promise { + if (!this.$hostInfo.isWindows) { + // when running under 'sudo' we create a working dir with wrong owner (root) and + // it is no longer accessible for the user initiating the installation + // patch the owner here + if (process.env.SUDO_USER) { + // TODO: Check if this is the correct place, probably we should set this at the end of the command. + await this.$fs.setCurrentUserAsOwner(this.$settingsService.getProfileDir(), process.env.SUDO_USER); + } + } + + await this.$helpService.generateHtmlPages(); + // Explicitly ask for confirmation of usage-reporting: + await this.$analyticsService.checkConsent(); + await this.$commandsService.tryExecuteCommand("autocomplete", []); + // Make sure the success message is separated with at least one line from all other messages. + this.$logger.out(); + this.$logger.printMarkdown("Installation successful. You are good to go. Connect with us on `http://twitter.com/NativeScript`."); await this.$subscriptionService.subscribeForNewsletter(); } diff --git a/lib/common/commands/post-install.ts b/lib/common/commands/post-install.ts index abd657299d..7b29f16870 100644 --- a/lib/common/commands/post-install.ts +++ b/lib/common/commands/post-install.ts @@ -1,38 +1,12 @@ export class PostInstallCommand implements ICommand { - constructor(private $fs: IFileSystem, - private $staticConfig: Config.IStaticConfig, - private $commandsService: ICommandsService, - private $helpService: IHelpService, - private $settingsService: ISettingsService, - private $analyticsService: IAnalyticsService, - protected $logger: ILogger) { + constructor(protected $errors: IErrors) { } public disableAnalytics = true; public allowedParameters: ICommandParameter[] = []; public async execute(args: string[]): Promise { - if (process.platform !== "win32") { - // when running under 'sudo' we create a working dir with wrong owner (root) and - // it is no longer accessible for the user initiating the installation - // patch the owner here - if (process.env.SUDO_USER) { - await this.$fs.setCurrentUserAsOwner(this.$settingsService.getProfileDir(), process.env.SUDO_USER); - } - } - - await this.$helpService.generateHtmlPages(); - - // Explicitly ask for confirmation of usage-reporting: - await this.$analyticsService.checkConsent(); - - await this.$commandsService.tryExecuteCommand("autocomplete", []); - - if (this.$staticConfig.INSTALLATION_SUCCESS_MESSAGE) { - // Make sure the success message is separated with at least one line from all other messages. - this.$logger.out(); - this.$logger.printMarkdown(this.$staticConfig.INSTALLATION_SUCCESS_MESSAGE); - } + this.$errors.fail("This command is deprecated. Use `tns dev-post-install-cli` instead"); } } $injector.registerCommand("dev-post-install", PostInstallCommand); diff --git a/lib/common/commands/preuninstall.ts b/lib/common/commands/preuninstall.ts index e5f08e14ea..41ba5c3c4e 100644 --- a/lib/common/commands/preuninstall.ts +++ b/lib/common/commands/preuninstall.ts @@ -1,4 +1,5 @@ import * as path from "path"; +import { doesCurrentNpmCommandMatch } from "../helpers"; export class PreUninstallCommand implements ICommand { public disableAnalytics = true; @@ -11,31 +12,14 @@ export class PreUninstallCommand implements ICommand { private $settingsService: ISettingsService) { } public async execute(args: string[]): Promise { - if (this.isIntentionalUninstall()) { + const isIntentionalUninstall = doesCurrentNpmCommandMatch([/^uninstall$/, /^remove$/, /^rm$/, /^r$/, /^un$/, /^unlink$/]); + if (isIntentionalUninstall) { this.handleIntentionalUninstall(); } this.$fs.deleteFile(path.join(this.$settingsService.getProfileDir(), "KillSwitches", "cli")); } - private isIntentionalUninstall(): boolean { - let isIntentionalUninstall = false; - if (process.env && process.env.npm_config_argv) { - try { - const npmConfigArgv = JSON.parse(process.env.npm_config_argv); - const uninstallAliases = ["uninstall", "remove", "rm", "r", "un", "unlink"]; - if (_.intersection(npmConfigArgv.original, uninstallAliases).length > 0) { - isIntentionalUninstall = true; - } - } catch (error) { - // ignore - } - - } - - return isIntentionalUninstall; - } - private handleIntentionalUninstall(): void { this.$extensibilityService.removeAllExtensions(); this.$packageInstallationManager.clearInspectorCache(); diff --git a/lib/common/definitions/config.d.ts b/lib/common/definitions/config.d.ts index e17f420e0a..f2de7adc6e 100644 --- a/lib/common/definitions/config.d.ts +++ b/lib/common/definitions/config.d.ts @@ -23,7 +23,6 @@ declare module Config { RESOURCE_DIR_PATH: string; PATH_TO_BOOTSTRAP: string; QR_SIZE: number; - INSTALLATION_SUCCESS_MESSAGE?: string; PROFILE_DIR_NAME: string } diff --git a/lib/common/helpers.ts b/lib/common/helpers.ts index 85117c7ea4..c47b089604 100644 --- a/lib/common/helpers.ts +++ b/lib/common/helpers.ts @@ -8,6 +8,62 @@ import * as crypto from "crypto"; const Table = require("cli-table"); +export function doesCurrentNpmCommandMatch(patterns?: RegExp[]): boolean { + const currentNpmCommandArgv = getCurrentNpmCommandArgv(); + let result = false; + + if (currentNpmCommandArgv.length) { + result = someWithRegExps(currentNpmCommandArgv, patterns); + } + + return result; +} + +/** + * Equivalent of lodash's some, but instead of lambda, just pass array of Regular Expressions. + * If any of them matches any of the given elements, true is returned. + * @param {string[]} array Elements to be checked. + * @param {RegExp[]} patterns Regular expressions to be tested + * @returns {boolean} True in case any element of the array matches any of the patterns. False otherwise. + */ +export function someWithRegExps(array: string[], patterns: RegExp[]): boolean { + return _.some(array, item => _.some(patterns, pattern => !!item.match(pattern))); +} + +export function getCurrentNpmCommandArgv(): string[] { + let result = []; + if (process.env && process.env.npm_config_argv) { + try { + const npmConfigArgv = JSON.parse(process.env.npm_config_argv); + result = npmConfigArgv.original || []; + } catch (error) { + // ignore + } + } + + return result; +} + +export function isInstallingNativeScriptGlobally(): boolean { + return isInstallingNativeScriptGloballyWithNpm() || isInstallingNativeScriptGloballyWithYarn(); +} + +function isInstallingNativeScriptGloballyWithNpm(): boolean { + const isInstallCommand = doesCurrentNpmCommandMatch([/^install$/, /^i$/]); + const isGlobalCommand = doesCurrentNpmCommandMatch([/^--global$/, /^-g$/]); + const hasNativeScriptPackage = doesCurrentNpmCommandMatch([/^nativescript(@.*)?$/]); + + return isInstallCommand && isGlobalCommand && hasNativeScriptPackage; +} + +function isInstallingNativeScriptGloballyWithYarn(): boolean { + // yarn populates the same env used by npm - npm_config_argv, so check it for yarn specific command + const isInstallCommand = doesCurrentNpmCommandMatch([/^add$/]); + const isGlobalCommand = doesCurrentNpmCommandMatch([/^global$/]); + const hasNativeScriptPackage = doesCurrentNpmCommandMatch([/^nativescript(@.*)?$/]); + + return isInstallCommand && isGlobalCommand && hasNativeScriptPackage; +} /** * Creates regular expression from input string. * The method replaces all occurences of RegExp special symbols in the input string with \. diff --git a/lib/common/test/unit-tests/helpers.ts b/lib/common/test/unit-tests/helpers.ts index d137caeca0..b7f3a1f34e 100644 --- a/lib/common/test/unit-tests/helpers.ts +++ b/lib/common/test/unit-tests/helpers.ts @@ -9,6 +9,14 @@ interface ITestData { } describe("helpers", () => { + let originalProcessEnvNpmConfig: any = null; + beforeEach(() => { + originalProcessEnvNpmConfig = process.env.npm_config_argv; + }); + + afterEach(() => { + process.env.npm_config_argv = originalProcessEnvNpmConfig; + }); const assertTestData = (testData: ITestData, method: Function) => { const actualResult = method(testData.input); @@ -699,4 +707,122 @@ describe("helpers", () => { }); }); }); + + const setNpmConfigArgv = (original: string[]): void => { + process.env.npm_config_argv = JSON.stringify({ original }); + }; + + describe("doesCurrentNpmCommandMatch", () => { + describe("when searching for global flag (--global or -g)", () => { + [ + { + name: "returns true when `--global` is passed on terminal", + input: ["install", "--global", "nativescript"], + expectedOutput: true + }, + { + name: "returns true when `-g` is passed on terminal", + input: ["install", "-g", "nativescript"], + expectedOutput: true + }, + { + name: "returns false neither -g/--global are passed on terminal", + input: ["install", "nativescript"], + expectedOutput: false + }, + { + name: "returns false when neither -g/--global are passed on terminal, but similar flag is passed", + input: ["install", "nativescript", "--globalEnv"], + expectedOutput: false + }, + { + name: "returns false when neither -g/--global are passed on terminal, but trying to install global package", + input: ["install", "global"], + expectedOutput: false + } + ].forEach(testCase => { + it(testCase.name, () => { + setNpmConfigArgv(testCase.input); + const result = helpers.doesCurrentNpmCommandMatch([/^--global$/, /^-g$/]); + assert.equal(result, testCase.expectedOutput); + }); + }); + }); + }); + + describe("isInstallingNativeScriptGlobally", () => { + const installationFlags = ["install", "i"]; + const globalFlags = ["--global", "-g"]; + const validNativeScriptPackageNames = ["nativescript", "nativescript@1.0.1", "nativescript@next"]; + + it("returns true when installing nativescript globally with npm", () => { + validNativeScriptPackageNames.forEach(nativescript => { + installationFlags.forEach(install => { + globalFlags.forEach(globalFlag => { + const npmArgs = [install, nativescript, globalFlag]; + setNpmConfigArgv(npmArgs); + const result = helpers.isInstallingNativeScriptGlobally(); + assert.isTrue(result); + }); + }); + }); + }); + + it("returns true when installing nativescript globally with yarn", () => { + validNativeScriptPackageNames.forEach(nativescript => { + const npmArgs = ["global", "add", nativescript]; + setNpmConfigArgv(npmArgs); + const result = helpers.isInstallingNativeScriptGlobally(); + assert.isTrue(result); + }); + }); + + const invalidInstallationFlags = ["installpackage", "is"]; + const invalidGlobalFlags = ["--globalEnv", ""]; + const invalidNativeScriptPackageNames = ["nativescript", "nativescript-facebook", "nativescript-facebook@1.0.1", "kinvey-nativescript-plugin"]; + + it(`returns false when command does not install nativescript globally`, () => { + invalidInstallationFlags.forEach(nativescript => { + invalidGlobalFlags.forEach(install => { + invalidNativeScriptPackageNames.forEach(globalFlag => { + const npmArgs = [install, nativescript, globalFlag]; + setNpmConfigArgv(npmArgs); + const result = helpers.isInstallingNativeScriptGlobally(); + assert.isFalse(result); + }); + }); + }); + }); + }); + + describe("getCurrentNpmCommandArgv", () => { + it("returns the value of process.env.npm_config_argv.original", () => { + const command = ["install", "nativescript"]; + process.env.npm_config_argv = JSON.stringify({ someOtherProp: 1, original: command }); + const actualCommand = helpers.getCurrentNpmCommandArgv(); + assert.deepEqual(actualCommand, command); + }); + + describe("returns empty array", () => { + const assertResultIsEmptyArray = () => { + const actualCommand = helpers.getCurrentNpmCommandArgv(); + assert.deepEqual(actualCommand, []); + }; + + it("when npm_config_argv is not populated", () => { + delete process.env.npm_config_argv; + assertResultIsEmptyArray(); + }); + + it("when npm_config_argv is not a valid json", () => { + process.env.npm_config_argv = "invalid datas"; + assertResultIsEmptyArray(); + }); + + it("when npm_config_argv.original is null", () => { + process.env.npm_config_argv = JSON.stringify({ original: null }); + assertResultIsEmptyArray(); + }); + }); + }); }); diff --git a/lib/config.ts b/lib/config.ts index a39ec8f83c..9c43b70a30 100644 --- a/lib/config.ts +++ b/lib/config.ts @@ -38,7 +38,6 @@ export class StaticConfig implements IStaticConfig { public TRACK_FEATURE_USAGE_SETTING_NAME = "TrackFeatureUsage"; public ERROR_REPORT_SETTING_NAME = "TrackExceptions"; public ANALYTICS_INSTALLATION_ID_SETTING_NAME = "AnalyticsInstallationID"; - public INSTALLATION_SUCCESS_MESSAGE = "Installation successful. You are good to go. Connect with us on `http://twitter.com/NativeScript`."; public get PROFILE_DIR_NAME(): string { return ".nativescript-cli"; } diff --git a/postinstall.js b/postinstall.js index 8c3a76fab2..3c7693699d 100644 --- a/postinstall.js +++ b/postinstall.js @@ -5,5 +5,7 @@ var path = require("path"); var constants = require(path.join(__dirname, "lib", "constants")); var commandArgs = [path.join(__dirname, "bin", "tns"), constants.POST_INSTALL_COMMAND_NAME]; var nodeArgs = require(path.join(__dirname, "lib", "common", "scripts", "node-args")).getNodeArgs(); - -child_process.spawn(process.argv[0], nodeArgs.concat(commandArgs), { stdio: "inherit" }); +var helpers = require(path.join(__dirname, "lib", "common", "helpers")); +if (helpers.isInstallingNativeScriptGlobally()) { + child_process.spawn(process.argv[0], nodeArgs.concat(commandArgs), { stdio: "inherit" }); +} diff --git a/test/commands/post-install.ts b/test/commands/post-install.ts index 78f0b04cea..ecfb2070f4 100644 --- a/test/commands/post-install.ts +++ b/test/commands/post-install.ts @@ -43,6 +43,8 @@ const createTestInjector = (): IInjector => { testInjector.registerCommand("post-install-cli", PostInstallCliCommand); + testInjector.register("hostInfo", {}); + return testInjector; }; diff --git a/test/post-install.ts b/test/post-install.ts index 7f2f98bd7a..11f5c6f8d0 100644 --- a/test/post-install.ts +++ b/test/post-install.ts @@ -2,26 +2,38 @@ import { assert } from "chai"; // Use require instead of import in order to replace the `spawn` method of child_process const childProcess = require("child_process"); +const helpers = require("../lib/common/helpers"); import { SpawnOptions, ChildProcess } from "child_process"; import * as path from "path"; import { POST_INSTALL_COMMAND_NAME } from "../lib/constants"; describe("postinstall.js", () => { - it("calls post-install-cli command of CLI", () => { - const originalSpawn = childProcess.spawn; - let isSpawnCalled = false; - let argsPassedToSpawn: string[] = []; + let isSpawnCalled = false; + let argsPassedToSpawn: string[] = []; + const originalSpawn = childProcess.spawn; + const originalIsInstallingNativeScriptGlobally = helpers.isInstallingNativeScriptGlobally; + + beforeEach(() => { + isSpawnCalled = false; + argsPassedToSpawn = []; childProcess.spawn = (command: string, args?: string[], options?: SpawnOptions): ChildProcess => { isSpawnCalled = true; argsPassedToSpawn = args; return null; }; + }); - require(path.join(__dirname, "..", "postinstall")); - + afterEach(() => { childProcess.spawn = originalSpawn; + helpers.isInstallingNativeScriptGlobally = originalIsInstallingNativeScriptGlobally; + }); + + it("calls post-install-cli command of CLI when it is global installation", () => { + helpers.isInstallingNativeScriptGlobally = () => true; + + require(path.join(__dirname, "..", "postinstall")); assert.isTrue(isSpawnCalled, "child_process.spawn must be called from postinstall.js"); @@ -32,4 +44,10 @@ describe("postinstall.js", () => { assert.isTrue(argsPassedToSpawn.indexOf(POST_INSTALL_COMMAND_NAME) !== -1, `The spawned args must contain the name of the post-install command. Expected path is: ${expectedPathToCliExecutable}, current args are: ${argsPassedToSpawn}.`); }); + + it("does not call post-install-cli command of CLI when it is not global install", () => { + helpers.isInstallingNativeScriptGlobally = () => false; + require(path.join(__dirname, "..", "postinstall")); + assert.isFalse(isSpawnCalled, "child_process.spawn must NOT be called from postinstall.js when CLI is not installed globally"); + }); }); From 3d13b995a55b1357c72ec948d4e5f2172cb214c0 Mon Sep 17 00:00:00 2001 From: rosen-vladimirov Date: Tue, 29 Jan 2019 18:27:29 +0200 Subject: [PATCH 2/2] fix: add require of lodash in helpers Currently the postinstall.js requires helpers and tries to execute some methods of it. However, CLI is not bootstraped and the global variable `_` is not set, so the action fails. Fix the code by adding require to `lodash` in the current file. The other option of fix is to remove the `helpers` require from postinstall script to `post-install-cli` command. However, the current check is if we should execute the postinstall actions - if we move the check to the command, the commands service will still execute the check and prompt the user to allow tracking, which is not a desired behavior. --- lib/common/helpers.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/common/helpers.ts b/lib/common/helpers.ts index c47b089604..1152026caf 100644 --- a/lib/common/helpers.ts +++ b/lib/common/helpers.ts @@ -5,6 +5,7 @@ import { ReadStream } from "tty"; import { Configurations } from "./constants"; import { EventEmitter } from "events"; import * as crypto from "crypto"; +import * as _ from "lodash"; const Table = require("cli-table");