From e0194cd5c42b8e384623e855bec74bbe61387c11 Mon Sep 17 00:00:00 2001 From: rosen-vladimirov Date: Thu, 20 Feb 2020 19:08:26 +0200 Subject: [PATCH 1/7] fix: pnpm support should work on macOS and Linux Currently we are unable to build applications on macOS and Linux when pnpm is set as package manager. The problem seems to be caused by the way pnpm produces files in node_modules and the way webpack expects them. However, the issue is resolved in case we do not pass `--preserve-symlinks` option to Node.js when starting the webpack process. According to docs (https://pnpm.js.org/en/faq) the preserve-symlinks does not work very well and pnpm does not require it. So, to resolve the issue, skip the `--preserve-symlinks` option when pnpm is used. To achieve this, introduce a new method in the packageManager that returns the package manager used for the current process. --- lib/constants.ts | 6 +++++ lib/declarations.d.ts | 6 +++++ lib/package-manager.ts | 14 ++++++++-- .../webpack/webpack-compiler-service.ts | 26 ++++++++++++++----- .../webpack/webpack-compiler-service.ts | 3 +++ 5 files changed, 47 insertions(+), 8 deletions(-) diff --git a/lib/constants.ts b/lib/constants.ts index ae885229f4..e9dcd6366d 100644 --- a/lib/constants.ts +++ b/lib/constants.ts @@ -415,3 +415,9 @@ export enum LoggerConfigData { } export const EMIT_APPENDER_EVENT_NAME = "logData"; + +export enum PackageManagers { + npm = "npm", + pnpm = "pnpm", + yarn = "yarn" +} diff --git a/lib/declarations.d.ts b/lib/declarations.d.ts index 9dacaa4111..47b945d5e4 100644 --- a/lib/declarations.d.ts +++ b/lib/declarations.d.ts @@ -76,6 +76,12 @@ interface INodePackageManager { } interface IPackageManager extends INodePackageManager { + /** + * Gets the name of the package manager used for the current process. + * It can be read from the user settings or by passing -- option. + */ + getPackageManagerName(): Promise; + /** * Gets the version corresponding to the tag for the package * @param {string} packageName The name of the package. diff --git a/lib/package-manager.ts b/lib/package-manager.ts index 6e9439f8fc..418919aa79 100644 --- a/lib/package-manager.ts +++ b/lib/package-manager.ts @@ -1,8 +1,10 @@ import { cache, exported, invokeInit } from './common/decorators'; import { performanceLog } from "./common/decorators"; +import { PackageManagers } from './constants'; export class PackageManager implements IPackageManager { private packageManager: INodePackageManager; + private _packageManagerName: string; constructor( private $errors: IErrors, @@ -19,6 +21,11 @@ export class PackageManager implements IPackageManager { this.packageManager = await this._determinePackageManager(); } + @invokeInit() + public async getPackageManagerName(): Promise { + return this._packageManagerName; + } + @exported("packageManager") @performanceLog() @invokeInit() @@ -97,11 +104,14 @@ export class PackageManager implements IPackageManager { this.$errors.fail(`Unable to read package manager config from user settings ${err}`); } - if (pm === 'yarn' || this.$options.yarn) { + if (pm === PackageManagers.yarn || this.$options.yarn) { + this._packageManagerName = PackageManagers.yarn; return this.$yarn; - } else if (pm === 'pnpm' || this.$options.pnpm) { + } else if (pm === PackageManagers.pnpm || this.$options.pnpm) { + this._packageManagerName = PackageManagers.pnpm; return this.$pnpm; } else { + this._packageManagerName = PackageManagers.npm; return this.$npm; } } diff --git a/lib/services/webpack/webpack-compiler-service.ts b/lib/services/webpack/webpack-compiler-service.ts index 3151f8d813..dd1e77c72e 100644 --- a/lib/services/webpack/webpack-compiler-service.ts +++ b/lib/services/webpack/webpack-compiler-service.ts @@ -3,7 +3,7 @@ import * as child_process from "child_process"; import * as semver from "semver"; import { EventEmitter } from "events"; import { performanceLog } from "../../common/decorators"; -import { WEBPACK_COMPILATION_COMPLETE, WEBPACK_PLUGIN_NAME } from "../../constants"; +import { WEBPACK_COMPILATION_COMPLETE, WEBPACK_PLUGIN_NAME, PackageManagers } from "../../constants"; export class WebpackCompilerService extends EventEmitter implements IWebpackCompilerService { private webpackProcesses: IDictionary = {}; @@ -18,6 +18,7 @@ export class WebpackCompilerService extends EventEmitter implements IWebpackComp private $logger: ILogger, private $mobileHelper: Mobile.IMobileHelper, private $cleanupService: ICleanupService, + private $packageManager: IPackageManager, private $packageInstallationManager: IPackageInstallationManager ) { super(); } @@ -155,6 +156,15 @@ export class WebpackCompilerService extends EventEmitter implements IWebpackComp } } + private async shouldUsePreserveSymlinksOption(projectData: IProjectData): Promise { + // pnpm does not require symlink (https://github.com/nodejs/node-eps/issues/46#issuecomment-277373566) + // and it also does not work in some cases. + // Check https://github.com/NativeScript/nativescript-cli/issues/5259 for more information + const currentPackageManager = await this.$packageManager.getPackageManagerName(); + const res = currentPackageManager !== PackageManagers.pnpm; + return res; + } + @performanceLog() private async startWebpackProcess(platformData: IPlatformData, projectData: IProjectData, prepareData: IPrepareData): Promise { if (!this.$fs.exists(projectData.webpackConfigPath)) { @@ -164,18 +174,22 @@ export class WebpackCompilerService extends EventEmitter implements IWebpackComp const envData = this.buildEnvData(platformData.platformNameLowerCase, projectData, prepareData); const envParams = await this.buildEnvCommandLineParams(envData, platformData, projectData, prepareData); const additionalNodeArgs = semver.major(process.version) <= 8 ? ["--harmony"] : []; + + if (await this.shouldUsePreserveSymlinksOption(projectData)) { + additionalNodeArgs.push("--preserve-symlinks"); + } + + if (process.arch === "x64") { + additionalNodeArgs.unshift("--max_old_space_size=4096"); + } + const args = [ ...additionalNodeArgs, - "--preserve-symlinks", path.join(projectData.projectDir, "node_modules", "webpack", "bin", "webpack.js"), `--config=${projectData.webpackConfigPath}`, ...envParams ]; - if (process.arch === "x64") { - args.unshift("--max_old_space_size=4096"); - } - if (prepareData.watch) { args.push("--watch"); } diff --git a/test/services/webpack/webpack-compiler-service.ts b/test/services/webpack/webpack-compiler-service.ts index 8da3183c24..a1539ae88f 100644 --- a/test/services/webpack/webpack-compiler-service.ts +++ b/test/services/webpack/webpack-compiler-service.ts @@ -13,6 +13,9 @@ function getAllEmittedFiles(hash: string) { function createTestInjector(): IInjector { const testInjector = new Yok(); + testInjector.register("packageManager", { + getPackageManagerName: async () => "npm" + }); testInjector.register("webpackCompilerService", WebpackCompilerService); testInjector.register("childProcess", {}); testInjector.register("hooksService", {}); From f2f714c9dbaacb77bfc92d8226cf36e7c4a75cf9 Mon Sep 17 00:00:00 2001 From: rosen-vladimirov Date: Thu, 20 Feb 2020 19:38:53 +0200 Subject: [PATCH 2/7] fix: CLI should print help of commands with dash in the name When trying to print the help of a default hierarchical command, CLI expects the format of the command name to be `|*`. However, some commands have dash in the name, like `tns package-manager get`. Fix the expectation of the help service. --- lib/common/services/help-service.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/common/services/help-service.ts b/lib/common/services/help-service.ts index 70edce02eb..60228b5327 100644 --- a/lib/common/services/help-service.ts +++ b/lib/common/services/help-service.ts @@ -183,7 +183,7 @@ export class HelpService implements IHelpService { private async convertCommandNameToFileName(commandData: ICommandData): Promise { let { commandName } = commandData; - const defaultCommandMatch = commandName && commandName.match(/(\w+?)\|\*/); + const defaultCommandMatch = commandName && commandName.match(/([\w-]+?)\|\*/); if (defaultCommandMatch) { this.$logger.trace("Default command found. Replace current command name '%s' with '%s'.", commandName, defaultCommandMatch[1]); commandName = defaultCommandMatch[1]; From 1e2d6ecde53b259b289837402022f8c5ff8ebcdf Mon Sep 17 00:00:00 2001 From: rosen-vladimirov Date: Thu, 20 Feb 2020 19:41:05 +0200 Subject: [PATCH 3/7] fix: help of package-manager commands does not list pnpm The markdown files of package-manager commands does not list pnpm as supported value. Add it where is required and add help file for package-manager command (without passig get to it). --- docs/man_pages/general/package-manager-get.md | 2 +- docs/man_pages/general/package-manager-set.md | 2 +- docs/man_pages/general/package-manager.md | 25 +++++++++++++++++++ 3 files changed, 27 insertions(+), 2 deletions(-) create mode 100644 docs/man_pages/general/package-manager.md diff --git a/docs/man_pages/general/package-manager-get.md b/docs/man_pages/general/package-manager-get.md index ba516da2a3..9a73a20eca 100644 --- a/docs/man_pages/general/package-manager-get.md +++ b/docs/man_pages/general/package-manager-get.md @@ -21,5 +21,5 @@ General | `$ tns package-manager get` Command | Description ----------|---------- -[package-manager-set](package-manager-set.html) | Enables the specified package manager for the NativeScript CLI. Supported values are npm and yarn. +[package-manager-set](package-manager-set.html) | Enables the specified package manager for the NativeScript CLI. Supported values are npm, yarn and pnpm. <% } %> \ No newline at end of file diff --git a/docs/man_pages/general/package-manager-set.md b/docs/man_pages/general/package-manager-set.md index 333ae3c93a..e30988b9af 100644 --- a/docs/man_pages/general/package-manager-set.md +++ b/docs/man_pages/general/package-manager-set.md @@ -17,7 +17,7 @@ General | `$ tns package-manager set ` ### Arguments -* `` is the name of the package manager. Supported values are npm and yarn. +* `` is the name of the package manager. Supported values are npm, yarn and pnpm. <% if(isHtml) { %> diff --git a/docs/man_pages/general/package-manager.md b/docs/man_pages/general/package-manager.md new file mode 100644 index 0000000000..9a73a20eca --- /dev/null +++ b/docs/man_pages/general/package-manager.md @@ -0,0 +1,25 @@ +<% if (isJekyll) { %>--- +title: tns package-manager get +position: 19 +---<% } %> + +# tns package-manager get + +### Description + +Prints the value of the current package manager. + +### Commands + +Usage | Synopsis +------|------- +General | `$ tns package-manager get` + +<% if(isHtml) { %> + +### Related Commands + +Command | Description +----------|---------- +[package-manager-set](package-manager-set.html) | Enables the specified package manager for the NativeScript CLI. Supported values are npm, yarn and pnpm. +<% } %> \ No newline at end of file From 9e037f88fbc0117a16075e9058258ae7e33c8e88 Mon Sep 17 00:00:00 2001 From: rosen-vladimirov Date: Thu, 20 Feb 2020 19:42:45 +0200 Subject: [PATCH 4/7] fix: register package-manager get as default hierarchical command Register the package-manager get as default hierarchical command, so when you write `tns package-manager` we'll print the currently selected package-manager. --- lib/bootstrap.ts | 2 +- lib/common/commands/package-manager-get.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/bootstrap.ts b/lib/bootstrap.ts index 865c99723b..3e51ddaed5 100644 --- a/lib/bootstrap.ts +++ b/lib/bootstrap.ts @@ -113,8 +113,8 @@ $injector.requirePublic("packageManager", "./package-manager"); $injector.requirePublic("npm", "./node-package-manager"); $injector.requirePublic("yarn", "./yarn-package-manager"); $injector.requirePublic("pnpm", "./pnpm-package-manager"); +$injector.requireCommand("package-manager|*get", "./commands/package-manager-get"); $injector.requireCommand("package-manager|set", "./commands/package-manager-set"); -$injector.requireCommand("package-manager|get", "./commands/package-manager-get"); $injector.require("packageInstallationManager", "./package-installation-manager"); diff --git a/lib/common/commands/package-manager-get.ts b/lib/common/commands/package-manager-get.ts index c6de826b6c..6d3f87e8d0 100644 --- a/lib/common/commands/package-manager-get.ts +++ b/lib/common/commands/package-manager-get.ts @@ -15,8 +15,8 @@ export class PackageManagerGetCommand implements ICommand { } const result = await this.$userSettingsService.getSettingValue("packageManager"); - this.$logger.info(`Your current package manager is ${result || "npm"}.`); + this.$logger.printMarkdown(`Your current package manager is \`${result || "npm"}\`.`); } } -$injector.registerCommand("package-manager|get", PackageManagerGetCommand); +$injector.registerCommand("package-manager|*get", PackageManagerGetCommand); From ebaab3cfa550c161468bbe8183bdc0955bd080a0 Mon Sep 17 00:00:00 2001 From: rosen-vladimirov Date: Thu, 20 Feb 2020 19:43:51 +0200 Subject: [PATCH 5/7] fix: print information message when package-manager is set Currently when calling `tns package-manager set ` you do not see any output or information if the operation is successful. Add such message and clean the code from hardcoded strings. --- lib/common/commands/package-manager-set.ts | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/lib/common/commands/package-manager-set.ts b/lib/common/commands/package-manager-set.ts index c9abe185a2..2025940c7d 100644 --- a/lib/common/commands/package-manager-set.ts +++ b/lib/common/commands/package-manager-set.ts @@ -1,21 +1,24 @@ +import { PackageManagers } from "../../constants"; export class PackageManagerCommand implements ICommand { constructor(private $userSettingsService: IUserSettingsService, private $errors: IErrors, + private $logger: ILogger, private $stringParameter: ICommandParameter) { } public allowedParameters: ICommandParameter[] = [this.$stringParameter]; - public execute(args: string[]): Promise { - if (args[0] === 'yarn') { - return this.$userSettingsService.saveSetting("packageManager", "yarn"); - } else if (args[0] === 'pnpm') { - return this.$userSettingsService.saveSetting("packageManager", "pnpm"); - } else if (args[0] === 'npm') { - return this.$userSettingsService.saveSetting("packageManager", "npm"); + public async execute(args: string[]): Promise { + const packageManagerName = args[0]; + const supportedPackageManagers = Object.keys(PackageManagers); + if (supportedPackageManagers.indexOf(packageManagerName) === -1) { + this.$errors.fail(`${packageManagerName} is not a valid package manager. Supported values are: ${supportedPackageManagers.join(", ")}.`); } - return this.$errors.fail(`${args[0]} is not a valid package manager. Only yarn or npm are supported.`); + + await this.$userSettingsService.saveSetting("packageManager", packageManagerName); + + this.$logger.printMarkdown(`You've successfully set \`${packageManagerName}\` as your package manager.`); } } From e7a96f6f85deed43951f05ee56c332a5f1bd11cf Mon Sep 17 00:00:00 2001 From: rosen-vladimirov Date: Thu, 20 Feb 2020 19:54:35 +0200 Subject: [PATCH 6/7] chore: update changelog for 6.4.1 --- CHANGELOG.md | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b6445ac82..bf20f9bd3f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,14 +1,24 @@ NativeScript CLI Changelog ================ -6.4.1 (2020, February 19) +6.4.1 (2020, February 21) === +### New + +* [Implemented #5255](https://github.com/NativeScript/nativescript-cli/issues/5255): Warn if the CLI might print sensitive data to the output + ### Fixed * [Fixed #5236](https://github.com/NativeScript/nativescript-cli/issues/5236): File paths from device logs are not clickable * [Fixed #5251](https://github.com/NativeScript/nativescript-cli/issues/5251): External files are not livesynced * [Fixed #5252](https://github.com/NativeScript/nativescript-cli/issues/5252): Logs from platform specific files point to incorrect file +* [Fixed #5259](https://github.com/NativeScript/nativescript-cli/issues/5259): Unable to use pnpm on macOS and Linux +* [Fixed #5260](https://github.com/NativeScript/nativescript-cli/issues/5260): `tns package-manager set invalid_value` does not say pnpm is supported +* [Fixed #5261](https://github.com/NativeScript/nativescript-cli/issues/5261): `tns package-manager set ` does not give any output +* [Fixed #5262](https://github.com/NativeScript/nativescript-cli/issues/5262): `tns package-manager` fails with error +* [Fixed #5263](https://github.com/NativeScript/nativescript-cli/issues/5263): `tns package-manager` docs does not list pnpm as supported value +* [Fixed #5264](https://github.com/NativeScript/nativescript-cli/issues/5264): `tns package-manager --help` fails with error 6.4.0 (2020, February 11) From 3b363eb9cbea6a34fd9523dcc52f34e178afc002 Mon Sep 17 00:00:00 2001 From: rosen-vladimirov Date: Fri, 21 Feb 2020 09:43:26 +0200 Subject: [PATCH 7/7] fix: remove unneeded method param --- lib/services/webpack/webpack-compiler-service.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/services/webpack/webpack-compiler-service.ts b/lib/services/webpack/webpack-compiler-service.ts index dd1e77c72e..d95e027c2b 100644 --- a/lib/services/webpack/webpack-compiler-service.ts +++ b/lib/services/webpack/webpack-compiler-service.ts @@ -156,7 +156,7 @@ export class WebpackCompilerService extends EventEmitter implements IWebpackComp } } - private async shouldUsePreserveSymlinksOption(projectData: IProjectData): Promise { + private async shouldUsePreserveSymlinksOption(): Promise { // pnpm does not require symlink (https://github.com/nodejs/node-eps/issues/46#issuecomment-277373566) // and it also does not work in some cases. // Check https://github.com/NativeScript/nativescript-cli/issues/5259 for more information @@ -175,7 +175,7 @@ export class WebpackCompilerService extends EventEmitter implements IWebpackComp const envParams = await this.buildEnvCommandLineParams(envData, platformData, projectData, prepareData); const additionalNodeArgs = semver.major(process.version) <= 8 ? ["--harmony"] : []; - if (await this.shouldUsePreserveSymlinksOption(projectData)) { + if (await this.shouldUsePreserveSymlinksOption()) { additionalNodeArgs.push("--preserve-symlinks"); }