Merge pull request #5265 from NativeScript/vladimirov/fix-pnpm

rosen-vladimirov · web-flow · commit 5ab0d306d481 · 2020-02-21T09:51:35.000+02:00
fix: package-manager and pnpm support fixes
diff --git 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 <valid value>` 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)
diff --git 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.
 <% } %>
diff --git 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 <PackageManager>`
 
 ### Arguments
 
-* `<PackageManager>` is the name of the package manager. Supported values are npm and yarn.
+* `<PackageManager>` 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
@@ -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.
+<% } %>
diff --git 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
@@ -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);
diff --git 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<void> {
-		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<void> {
+		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.`);
 	}
 }
 
diff --git 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<string> {
 		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];
diff --git 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
@@ -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<string>;
+
 	/**
 	 * 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
@@ -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<string> {
+		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
@@ -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<child_process.ChildProcess> = {};
@@ -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(): Promise<boolean> {
+		// 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<child_process.ChildProcess> {
 		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()) {
+			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
@@ -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", {});

Original file line number	Diff line number	Diff line change
`@@ -15,8 +15,8 @@ export class PackageManagerGetCommand implements ICommand {`
`15`	`15`	`}`
`16`	`16`
`17`	`17`	`const result = await this.$userSettingsService.getSettingValue("packageManager");`
`18`		- this.$logger.info(`Your current package manager is ${result \|\| "npm"}.`);
	`18`	+ this.$logger.printMarkdown(`Your current package manager is \`${result \|\| "npm"}\`.`);
`19`	`19`	`}`
`20`	`20`	`}`
`21`	`21`
`22`		`-$injector.registerCommand("package-manager\|get", PackageManagerGetCommand);`
	`22`	`+$injector.registerCommand("package-manager\|*get", PackageManagerGetCommand);`
Original file line number	Diff line number	Diff line change
`@@ -415,3 +415,9 @@ export enum LoggerConfigData {`
`415`	`415`	`}`
`416`	`416`
`417`	`417`	`export const EMIT_APPENDER_EVENT_NAME = "logData";`
	`418`	`+`
	`419`	`+export enum PackageManagers {`
	`420`	`+ npm = "npm",`
	`421`	`+ pnpm = "pnpm",`
	`422`	`+ yarn = "yarn"`
	`423`	`+}`