Fix getting production dependencies code

CLI has logic to find which are the "production" dependencies, i.e. which should be copied from `node_modules` to `platforms` dir. However, when the project has a lot of dependencies (more than 15), on some machines the code leads to error: "Maximum callstack size exceeded". On other machines the code tooks significant time to execute. After investigation, it turned out the recursion inside `node-modules-dependencies-builder` is incorrect and it adds each package many times to the result array. Fix the recursion and change the class NodeModulesDependenciesBuilder to be stateless - instead of using properties in `this` object when calculating the production dependencies, the methods will persist the results through the passed args. This way the whole class can be safely added to `$injector` and used whenever we need the production dependencies. Each time the calculation is started from the beginning, which is the requirement for long living process, where the project may change. Fix the type of the result, which leads to fix in several other services, where the result has been expected as `IDictionary<smth>`. However it's never been dictionary, it's always been an array. The code, that expected dictionary has been working because the `_.values` method of lodash (used all over the places where the incorrect type of data has been expected), returns the same array when the passed argument is array. Fix the tests that incorrectly expected dictionary with keys "0", "1", "2", etc. Remove the usage of Node.js's `fs` module from `NodeModulesDependenciesBuilder` - replace it with `$fs` which allows easir writing of tests. Require the `nodeModulesDependenciesBuilder` in bootstrap, so it can be correctly resolved by `$injector`. Add unit tests for `nodeModulesDependenciesBuilder`.
NativeScript · May 7, 2017 · 9d03f07 · 9d03f07
1 parent c0be28f
commit 9d03f07
Show file tree

Hide file tree

Showing 15 changed files with 451 additions and 100 deletions.
diff --git a/lib/bootstrap.ts b/lib/bootstrap.ts
@@ -135,3 +135,5 @@ $injector.requireCommand("extension|*list", "./commands/extensibility/list-exten
 $injector.requireCommand("extension|install", "./commands/extensibility/install-extension");
 $injector.requireCommand("extension|uninstall", "./commands/extensibility/uninstall-extension");
 $injector.requirePublic("extensibilityService", "./services/extensibility-service");
+
+$injector.require("nodeModulesDependenciesBuilder", "./tools/node-modules/node-modules-dependencies-builder");
diff --git a/lib/common b/lib/common
diff --git a/lib/declarations.d.ts b/lib/declarations.d.ts
@@ -202,11 +202,45 @@ interface INpmInstallOptions {
 	dependencyType?: string;
 }
 
+/**
+ * Describes a package installed in node_modules directory of a project.
+ */
 interface IDependencyData {
+	/**
+	 * The name of the package.
+	 */
 	name: string;
-	version: string;
-	nativescript: any;
+
+	/**
+	 * Version of the dependency.
+	 */
+	version?: string;
+
+	/**
+	 * The full path where the package is installed.
+	 */
+	directory: string;
+
+	/**
+	 * The depth inside node_modules dir, where the package is located.
+	 * The <project_dir>/node_modules/ is level 0.
+	 * Level 1 is <project dir>/node_modules/<package name>/node_modules, etc.
+	 */
+	depth: number;
+
+	/**
+	 * Describes the `nativescript` key in package.json of a dependency.
+	 */
+	nativescript?: any;
+
+	/**
+	 * Dependencies of the project - the value of dependencies key in project's package.json.
+	 */
 	dependencies?: IStringDictionary;
+
+	/**
+	 * Dev Dependencies of the project - the value of devDependencies key in project's package.json.
+	 */
 	devDependencies?: IStringDictionary;
 }
 

diff --git a/lib/definitions/platform.d.ts b/lib/definitions/platform.d.ts
@@ -275,7 +275,7 @@ interface INodeModulesBuilder {
 }
 
 interface INodeModulesDependenciesBuilder {
-	getProductionDependencies(projectPath: string): void;
+	getProductionDependencies(projectPath: string): IDependencyData[];
 }
 
 interface IBuildInfo {

diff --git a/lib/definitions/project.d.ts b/lib/definitions/project.d.ts
@@ -225,7 +225,7 @@ interface IPlatformProjectService extends NodeJS.EventEmitter {
 	removePluginNativeCode(pluginData: IPluginData, projectData: IProjectData): Promise<void>;
 
 	afterPrepareAllPlugins(projectData: IProjectData): Promise<void>;
-	beforePrepareAllPlugins(projectData: IProjectData, dependencies?: IDictionary<IDependencyData>): Promise<void>;
+	beforePrepareAllPlugins(projectData: IProjectData, dependencies?: IDependencyData[]): Promise<void>;
 
 	/**
 	 * Gets the path wheren App_Resources should be copied.

diff --git a/lib/services/android-project-service.ts b/lib/services/android-project-service.ts
@@ -400,7 +400,7 @@ export class AndroidProjectService extends projectServiceBaseLib.PlatformProject
 		return;
 	}
 
-	public async beforePrepareAllPlugins(projectData: IProjectData, dependencies?: IDictionary<IDependencyData>): Promise<void> {
+	public async beforePrepareAllPlugins(projectData: IProjectData, dependencies?: IDependencyData[]): Promise<void> {
 		if (!this.$config.debugLivesync) {
 			if (dependencies) {
 				let platformDir = path.join(projectData.platformsDir, "android");

diff --git a/lib/services/livesync/livesync-service.ts b/lib/services/livesync/livesync-service.ts
@@ -1,7 +1,6 @@
 import * as constants from "../../constants";
 import * as helpers from "../../common/helpers";
 import * as path from "path";
-import { NodeModulesDependenciesBuilder } from "../../tools/node-modules/node-modules-dependencies-builder";
 
 let choki = require("chokidar");
 
@@ -17,7 +16,8 @@ class LiveSyncService implements ILiveSyncService {
 		private $logger: ILogger,
 		private $dispatcher: IFutureDispatcher,
 		private $hooksService: IHooksService,
-		private $processService: IProcessService) { }
+		private $processService: IProcessService,
+		private $nodeModulesDependenciesBuilder: INodeModulesDependenciesBuilder) { }
 
 	public get isInitialized(): boolean { // This function is used from https://github.com/NativeScript/nativescript-dev-typescript/blob/master/lib/before-prepare.js#L4
 		return this._isInitialized;
@@ -94,8 +94,7 @@ class LiveSyncService implements ILiveSyncService {
 
 	private partialSync(syncWorkingDirectory: string, onChangedActions: ((event: string, filePath: string, dispatcher: IFutureDispatcher) => Promise<void>)[], projectData: IProjectData): void {
 		let that = this;
-		let dependenciesBuilder = this.$injector.resolve(NodeModulesDependenciesBuilder, {});
-		let productionDependencies = dependenciesBuilder.getProductionDependencies(projectData.projectDir);
+		let productionDependencies = this.$nodeModulesDependenciesBuilder.getProductionDependencies(projectData.projectDir);
 		let pattern = ["app"];
 
 		if (this.$options.syncAllFiles) {

diff --git a/lib/tools/node-modules/node-modules-builder.ts b/lib/tools/node-modules/node-modules-builder.ts
@@ -1,11 +1,11 @@
 import * as shelljs from "shelljs";
 import { TnsModulesCopy, NpmPluginPrepare } from "./node-modules-dest-copy";
-import { NodeModulesDependenciesBuilder } from "./node-modules-dependencies-builder";
 
 export class NodeModulesBuilder implements INodeModulesBuilder {
 	constructor(private $fs: IFileSystem,
 		private $injector: IInjector,
-		private $options: IOptions
+		private $options: IOptions,
+		private $nodeModulesDependenciesBuilder: INodeModulesDependenciesBuilder
 	) { }
 
 	public async prepareNodeModules(absoluteOutputPath: string, platform: string, lastModifiedTime: Date, projectData: IProjectData): Promise<void> {
@@ -14,8 +14,7 @@ export class NodeModulesBuilder implements INodeModulesBuilder {
 			lastModifiedTime = null;
 		}
 
-		let dependenciesBuilder = this.$injector.resolve(NodeModulesDependenciesBuilder, {});
-		let productionDependencies = dependenciesBuilder.getProductionDependencies(projectData.projectDir);
+		let productionDependencies = this.$nodeModulesDependenciesBuilder.getProductionDependencies(projectData.projectDir);
 
 		if (!this.$options.bundle) {
 			const tnsModulesCopy = this.$injector.resolve(TnsModulesCopy, {

diff --git a/lib/tools/node-modules/node-modules-dependencies-builder.ts b/lib/tools/node-modules/node-modules-dependencies-builder.ts
@@ -1,77 +1,63 @@
 import * as path from "path";
-import * as fs from "fs";
+import { NODE_MODULES_FOLDER_NAME, PACKAGE_JSON_FILE_NAME } from "../../constants";
 
 export class NodeModulesDependenciesBuilder implements INodeModulesDependenciesBuilder {
-	private projectPath: string;
-	private rootNodeModulesPath: string;
-	private resolvedDependencies: any[];
-	private seen: any;
-
-	public constructor(private $fs: IFileSystem) {
-		this.seen = {};
-		this.resolvedDependencies = [];
-	}
+	public constructor(private $fs: IFileSystem) { }
 
-	public getProductionDependencies(projectPath: string): any[] {
-		this.projectPath = projectPath;
-		this.rootNodeModulesPath = path.join(this.projectPath, "node_modules");
+	public getProductionDependencies(projectPath: string): IDependencyData[] {
+		const rootNodeModulesPath = path.join(projectPath, NODE_MODULES_FOLDER_NAME);
+		const projectPackageJsonPath = path.join(projectPath, PACKAGE_JSON_FILE_NAME);
+		const packageJsonContent = this.$fs.readJson(projectPackageJsonPath);
+		const dependencies = packageJsonContent && packageJsonContent.dependencies;
 
-		let projectPackageJsonpath = path.join(this.projectPath, "package.json");
-		let packageJsonContent = this.$fs.readJson(projectPackageJsonpath);
+		let resolvedDependencies: IDependencyData[] = [];
 
-		_.keys(packageJsonContent.dependencies).forEach(dependencyName => {
-			let depth = 0;
-			let directory = path.join(this.rootNodeModulesPath, dependencyName);
+		_.keys(dependencies)
+			.forEach(dependencyName => {
+				const depth = 0;
+				const directory = path.join(rootNodeModulesPath, dependencyName);
 
-			// find and traverse child with name `key`, parent's directory -> dep.directory
-			this.traverseDependency(dependencyName, directory, depth);
-		});
+				// find and traverse child with name `key`, parent's directory -> dep.directory
+				this.traverseDependency(resolvedDependencies, rootNodeModulesPath, dependencyName, directory, depth);
+			});
 
-		return this.resolvedDependencies;
+		return resolvedDependencies;
 	}
 
-	private traverseDependency(name: string, currentModulePath: string, depth: number): void {
+	private traverseDependency(resolvedDependencies: IDependencyData[], rootNodeModulesPath: string, name: string, currentModulePath: string, depth: number): void {
 		// Check if child has been extracted in the parent's node modules, AND THEN in `node_modules`
 		// Slower, but prevents copying wrong versions if multiple of the same module are installed
 		// Will also prevent copying project's devDependency's version if current module depends on another version
-		let modulePath = path.join(currentModulePath, "node_modules", name); // node_modules/parent/node_modules/<package>
-		let alternativeModulePath = path.join(this.rootNodeModulesPath, name);
+		const modulePath = path.join(currentModulePath, NODE_MODULES_FOLDER_NAME, name); // node_modules/parent/node_modules/<package>
+		const alternativeModulePath = path.join(rootNodeModulesPath, name);
 
-		this.findModule(modulePath, alternativeModulePath, name, depth);
+		this.findModule(resolvedDependencies, rootNodeModulesPath, modulePath, alternativeModulePath, name, depth);
 	}
 
-	private findModule(modulePath: string, alternativeModulePath: string, name: string, depth: number) {
+	private findModule(resolvedDependencies: IDependencyData[], rootNodeModulesPath: string, modulePath: string, rootModulesPath: string, name: string, depth: number): void {
 		let exists = this.moduleExists(modulePath);
+		let depthInNodeModules = depth + 1;
 
-		if (exists) {
-			if (this.seen[modulePath]) {
-				return;
-			}
-
-			let dependency = this.addDependency(name, modulePath, depth + 1);
-			this.readModuleDependencies(modulePath, depth + 1, dependency);
-		} else {
-			modulePath = alternativeModulePath; // /node_modules/<package>
+		if (!exists) {
+			modulePath = rootModulesPath; // /node_modules/<package>
 			exists = this.moduleExists(modulePath);
 
-			if (!exists) {
-				return;
-			}
-
-			if (this.seen[modulePath]) {
-				return;
-			}
+			depthInNodeModules = 0;
+		}
 
-			let dependency = this.addDependency(name, modulePath, 0);
-			this.readModuleDependencies(modulePath, 0, dependency);
+		if (!exists || _.some(resolvedDependencies, deps => deps.directory === modulePath)) {
+			return;
 		}
 
-		this.seen[modulePath] = true;
+		const dependency = this.getDependencyInfo(name, modulePath, depthInNodeModules);
+		resolvedDependencies.push(dependency);
+
+		this.readModuleDependencies(resolvedDependencies, rootNodeModulesPath, modulePath, depthInNodeModules, dependency);
 	}
 
-	private readModuleDependencies(modulePath: string, depth: number, currentModule: any): void {
-		let packageJsonPath = path.join(modulePath, 'package.json');
-		let packageJsonExists = fs.lstatSync(packageJsonPath).isFile();
+	private readModuleDependencies(resolvedDependencies: IDependencyData[], rootNodeModulesPath: string, modulePath: string, depth: number, currentModule: IDependencyData): void {
+		const packageJsonPath = path.join(modulePath, PACKAGE_JSON_FILE_NAME);
+		const packageJsonExists = this.$fs.getLsStats(packageJsonPath).isFile();
 
 		if (packageJsonExists) {
 			let packageJsonContents = this.$fs.readJson(packageJsonPath);
@@ -81,31 +67,31 @@ export class NodeModulesDependenciesBuilder implements INodeModulesDependenciesB
 				currentModule.nativescript = packageJsonContents.nativescript;
 			}
 
-			_.keys(packageJsonContents.dependencies).forEach((dependencyName) => {
-				this.traverseDependency(dependencyName, modulePath, depth);
-			});
+			_.keys(packageJsonContents.dependencies)
+				.forEach((dependencyName) => {
+					this.traverseDependency(resolvedDependencies, rootNodeModulesPath, dependencyName, modulePath, depth);
+				});
 		}
 	}
 
-	private addDependency(name: string, directory: string, depth: number): any {
-		let dependency: any = {
+	private getDependencyInfo(name: string, directory: string, depth: number): IDependencyData {
+		const dependency: IDependencyData = {
 			name,
 			directory,
 			depth
 		};
 
-		this.resolvedDependencies.push(dependency);
-
 		return dependency;
 	}
 
 	private moduleExists(modulePath: string): boolean {
 		try {
-			let exists = fs.lstatSync(modulePath);
-			if (exists.isSymbolicLink()) {
-				exists = fs.lstatSync(fs.realpathSync(modulePath));
+			let modulePathLsStat = this.$fs.getLsStats(modulePath);
+			if (modulePathLsStat.isSymbolicLink()) {
+				modulePathLsStat = this.$fs.getLsStats(this.$fs.realpath(modulePath));
 			}
-			return exists.isDirectory();
+
+			return modulePathLsStat.isDirectory();
 		} catch (e) {
 			return false;
 		}

diff --git a/lib/tools/node-modules/node-modules-dest-copy.ts b/lib/tools/node-modules/node-modules-dest-copy.ts
@@ -61,18 +61,18 @@ export class NpmPluginPrepare {
 	) {
 	}
 
-	protected async beforePrepare(dependencies: IDictionary<IDependencyData>, platform: string, projectData: IProjectData): Promise<void> {
+	protected async beforePrepare(dependencies: IDependencyData[], platform: string, projectData: IProjectData): Promise<void> {
 		await this.$platformsData.getPlatformData(platform, projectData).platformProjectService.beforePrepareAllPlugins(projectData, dependencies);
 	}
 
-	protected async afterPrepare(dependencies: IDictionary<IDependencyData>, platform: string, projectData: IProjectData): Promise<void> {
+	protected async afterPrepare(dependencies: IDependencyData[], platform: string, projectData: IProjectData): Promise<void> {
 		await this.$platformsData.getPlatformData(platform, projectData).platformProjectService.afterPrepareAllPlugins(projectData);
 		this.writePreparedDependencyInfo(dependencies, platform, projectData);
 	}
 
-	private writePreparedDependencyInfo(dependencies: IDictionary<IDependencyData>, platform: string, projectData: IProjectData): void {
+	private writePreparedDependencyInfo(dependencies: IDependencyData[], platform: string, projectData: IProjectData): void {
 		let prepareData: IDictionary<boolean> = {};
-		_.values(dependencies).forEach(d => {
+		_.each(dependencies, d => {
 			prepareData[d.name] = true;
 		});
 		this.$fs.createDirectory(this.preparedPlatformsDir(platform, projectData));
@@ -101,18 +101,18 @@ export class NpmPluginPrepare {
 		return this.$fs.readJson(this.preparedPlatformsFile(platform, projectData), "utf8");
 	}
 
-	private allPrepared(dependencies: IDictionary<IDependencyData>, platform: string, projectData: IProjectData): boolean {
+	private allPrepared(dependencies: IDependencyData[], platform: string, projectData: IProjectData): boolean {
 		let result = true;
 		const previouslyPrepared = this.getPreviouslyPreparedDependencies(platform, projectData);
-		_.values(dependencies).forEach(d => {
+		_.each(dependencies, d => {
 			if (!previouslyPrepared[d.name]) {
 				result = false;
 			}
 		});
 		return result;
 	}
 
-	public async preparePlugins(dependencies: IDictionary<IDependencyData>, platform: string, projectData: IProjectData): Promise<void> {
+	public async preparePlugins(dependencies: IDependencyData[], platform: string, projectData: IProjectData): Promise<void> {
 		if (_.isEmpty(dependencies) || this.allPrepared(dependencies, platform, projectData)) {
 			return;
 		}

diff --git a/test/npm-support.ts b/test/npm-support.ts
@@ -27,6 +27,7 @@ import { XmlValidator } from "../lib/xml-validator";
 import { LockFile } from "../lib/lockfile";
 import ProjectChangesLib = require("../lib/services/project-changes-service");
 import { Messages } from "../lib/common/messages/messages";
+import { NodeModulesDependenciesBuilder } from "../lib/tools/node-modules/node-modules-dependencies-builder";
 
 import path = require("path");
 import temp = require("temp");
@@ -81,9 +82,10 @@ function createTestInjector(): IInjector {
 	testInjector.register("projectChangesService", ProjectChangesLib.ProjectChangesService);
 	testInjector.register("emulatorPlatformService", stubs.EmulatorPlatformService);
 	testInjector.register("analyticsService", {
-		track: async () => undefined
+		track: async (): Promise<any> => undefined
 	});
 	testInjector.register("messages", Messages);
+	testInjector.register("nodeModulesDependenciesBuilder", NodeModulesDependenciesBuilder);
 
 	return testInjector;
 }