Skip to content

Commit 152c420

Browse files
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`.
1 parent 2124b88 commit 152c420

15 files changed

+451
-100
lines changed

lib/bootstrap.ts

+2
Original file line numberDiff line numberDiff line change
@@ -135,3 +135,5 @@ $injector.requireCommand("extension|*list", "./commands/extensibility/list-exten
135135
$injector.requireCommand("extension|install", "./commands/extensibility/install-extension");
136136
$injector.requireCommand("extension|uninstall", "./commands/extensibility/uninstall-extension");
137137
$injector.requirePublic("extensibilityService", "./services/extensibility-service");
138+
139+
$injector.require("nodeModulesDependenciesBuilder", "./tools/node-modules/node-modules-dependencies-builder");

lib/common

lib/declarations.d.ts

+36-2
Original file line numberDiff line numberDiff line change
@@ -202,11 +202,45 @@ interface INpmInstallOptions {
202202
dependencyType?: string;
203203
}
204204

205+
/**
206+
* Describes a package installed in node_modules directory of a project.
207+
*/
205208
interface IDependencyData {
209+
/**
210+
* The name of the package.
211+
*/
206212
name: string;
207-
version: string;
208-
nativescript: any;
213+
214+
/**
215+
* Version of the dependency.
216+
*/
217+
version?: string;
218+
219+
/**
220+
* The full path where the package is installed.
221+
*/
222+
directory: string;
223+
224+
/**
225+
* The depth inside node_modules dir, where the package is located.
226+
* The <project_dir>/node_modules/ is level 0.
227+
* Level 1 is <project dir>/node_modules/<package name>/node_modules, etc.
228+
*/
229+
depth: number;
230+
231+
/**
232+
* Describes the `nativescript` key in package.json of a dependency.
233+
*/
234+
nativescript?: any;
235+
236+
/**
237+
* Dependencies of the project - the value of dependencies key in project's package.json.
238+
*/
209239
dependencies?: IStringDictionary;
240+
241+
/**
242+
* Dev Dependencies of the project - the value of devDependencies key in project's package.json.
243+
*/
210244
devDependencies?: IStringDictionary;
211245
}
212246

lib/definitions/platform.d.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -275,7 +275,7 @@ interface INodeModulesBuilder {
275275
}
276276

277277
interface INodeModulesDependenciesBuilder {
278-
getProductionDependencies(projectPath: string): void;
278+
getProductionDependencies(projectPath: string): IDependencyData[];
279279
}
280280

281281
interface IBuildInfo {

lib/definitions/project.d.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -225,7 +225,7 @@ interface IPlatformProjectService extends NodeJS.EventEmitter {
225225
removePluginNativeCode(pluginData: IPluginData, projectData: IProjectData): Promise<void>;
226226

227227
afterPrepareAllPlugins(projectData: IProjectData): Promise<void>;
228-
beforePrepareAllPlugins(projectData: IProjectData, dependencies?: IDictionary<IDependencyData>): Promise<void>;
228+
beforePrepareAllPlugins(projectData: IProjectData, dependencies?: IDependencyData[]): Promise<void>;
229229

230230
/**
231231
* Gets the path wheren App_Resources should be copied.

lib/services/android-project-service.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -400,7 +400,7 @@ export class AndroidProjectService extends projectServiceBaseLib.PlatformProject
400400
return;
401401
}
402402

403-
public async beforePrepareAllPlugins(projectData: IProjectData, dependencies?: IDictionary<IDependencyData>): Promise<void> {
403+
public async beforePrepareAllPlugins(projectData: IProjectData, dependencies?: IDependencyData[]): Promise<void> {
404404
if (!this.$config.debugLivesync) {
405405
if (dependencies) {
406406
let platformDir = path.join(projectData.platformsDir, "android");

lib/services/livesync/livesync-service.ts

+3-4
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
import * as constants from "../../constants";
22
import * as helpers from "../../common/helpers";
33
import * as path from "path";
4-
import { NodeModulesDependenciesBuilder } from "../../tools/node-modules/node-modules-dependencies-builder";
54

65
let choki = require("chokidar");
76

@@ -17,7 +16,8 @@ class LiveSyncService implements ILiveSyncService {
1716
private $logger: ILogger,
1817
private $dispatcher: IFutureDispatcher,
1918
private $hooksService: IHooksService,
20-
private $processService: IProcessService) { }
19+
private $processService: IProcessService,
20+
private $nodeModulesDependenciesBuilder: INodeModulesDependenciesBuilder) { }
2121

2222
public get isInitialized(): boolean { // This function is used from https://git.1-hub.cnNativeScript/nativescript-dev-typescript/blob/master/lib/before-prepare.js#L4
2323
return this._isInitialized;
@@ -94,8 +94,7 @@ class LiveSyncService implements ILiveSyncService {
9494

9595
private partialSync(syncWorkingDirectory: string, onChangedActions: ((event: string, filePath: string, dispatcher: IFutureDispatcher) => Promise<void>)[], projectData: IProjectData): void {
9696
let that = this;
97-
let dependenciesBuilder = this.$injector.resolve(NodeModulesDependenciesBuilder, {});
98-
let productionDependencies = dependenciesBuilder.getProductionDependencies(projectData.projectDir);
97+
let productionDependencies = this.$nodeModulesDependenciesBuilder.getProductionDependencies(projectData.projectDir);
9998
let pattern = ["app"];
10099

101100
if (this.$options.syncAllFiles) {

lib/tools/node-modules/node-modules-builder.ts

+3-4
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
11
import * as shelljs from "shelljs";
22
import { TnsModulesCopy, NpmPluginPrepare } from "./node-modules-dest-copy";
3-
import { NodeModulesDependenciesBuilder } from "./node-modules-dependencies-builder";
43

54
export class NodeModulesBuilder implements INodeModulesBuilder {
65
constructor(private $fs: IFileSystem,
76
private $injector: IInjector,
8-
private $options: IOptions
7+
private $options: IOptions,
8+
private $nodeModulesDependenciesBuilder: INodeModulesDependenciesBuilder
99
) { }
1010

1111
public async prepareNodeModules(absoluteOutputPath: string, platform: string, lastModifiedTime: Date, projectData: IProjectData): Promise<void> {
@@ -14,8 +14,7 @@ export class NodeModulesBuilder implements INodeModulesBuilder {
1414
lastModifiedTime = null;
1515
}
1616

17-
let dependenciesBuilder = this.$injector.resolve(NodeModulesDependenciesBuilder, {});
18-
let productionDependencies = dependenciesBuilder.getProductionDependencies(projectData.projectDir);
17+
let productionDependencies = this.$nodeModulesDependenciesBuilder.getProductionDependencies(projectData.projectDir);
1918

2019
if (!this.$options.bundle) {
2120
const tnsModulesCopy = this.$injector.resolve(TnsModulesCopy, {

lib/tools/node-modules/node-modules-dependencies-builder.ts

+46-60
Original file line numberDiff line numberDiff line change
@@ -1,77 +1,63 @@
11
import * as path from "path";
2-
import * as fs from "fs";
2+
import { NODE_MODULES_FOLDER_NAME, PACKAGE_JSON_FILE_NAME } from "../../constants";
33

44
export class NodeModulesDependenciesBuilder implements INodeModulesDependenciesBuilder {
5-
private projectPath: string;
6-
private rootNodeModulesPath: string;
7-
private resolvedDependencies: any[];
8-
private seen: any;
9-
10-
public constructor(private $fs: IFileSystem) {
11-
this.seen = {};
12-
this.resolvedDependencies = [];
13-
}
5+
public constructor(private $fs: IFileSystem) { }
146

15-
public getProductionDependencies(projectPath: string): any[] {
16-
this.projectPath = projectPath;
17-
this.rootNodeModulesPath = path.join(this.projectPath, "node_modules");
7+
public getProductionDependencies(projectPath: string): IDependencyData[] {
8+
const rootNodeModulesPath = path.join(projectPath, NODE_MODULES_FOLDER_NAME);
9+
const projectPackageJsonPath = path.join(projectPath, PACKAGE_JSON_FILE_NAME);
10+
const packageJsonContent = this.$fs.readJson(projectPackageJsonPath);
11+
const dependencies = packageJsonContent && packageJsonContent.dependencies;
1812

19-
let projectPackageJsonpath = path.join(this.projectPath, "package.json");
20-
let packageJsonContent = this.$fs.readJson(projectPackageJsonpath);
13+
let resolvedDependencies: IDependencyData[] = [];
2114

22-
_.keys(packageJsonContent.dependencies).forEach(dependencyName => {
23-
let depth = 0;
24-
let directory = path.join(this.rootNodeModulesPath, dependencyName);
15+
_.keys(dependencies)
16+
.forEach(dependencyName => {
17+
const depth = 0;
18+
const directory = path.join(rootNodeModulesPath, dependencyName);
2519

26-
// find and traverse child with name `key`, parent's directory -> dep.directory
27-
this.traverseDependency(dependencyName, directory, depth);
28-
});
20+
// find and traverse child with name `key`, parent's directory -> dep.directory
21+
this.traverseDependency(resolvedDependencies, rootNodeModulesPath, dependencyName, directory, depth);
22+
});
2923

30-
return this.resolvedDependencies;
24+
return resolvedDependencies;
3125
}
3226

33-
private traverseDependency(name: string, currentModulePath: string, depth: number): void {
27+
private traverseDependency(resolvedDependencies: IDependencyData[], rootNodeModulesPath: string, name: string, currentModulePath: string, depth: number): void {
3428
// Check if child has been extracted in the parent's node modules, AND THEN in `node_modules`
3529
// Slower, but prevents copying wrong versions if multiple of the same module are installed
3630
// Will also prevent copying project's devDependency's version if current module depends on another version
37-
let modulePath = path.join(currentModulePath, "node_modules", name); // node_modules/parent/node_modules/<package>
38-
let alternativeModulePath = path.join(this.rootNodeModulesPath, name);
31+
const modulePath = path.join(currentModulePath, NODE_MODULES_FOLDER_NAME, name); // node_modules/parent/node_modules/<package>
32+
const alternativeModulePath = path.join(rootNodeModulesPath, name);
3933

40-
this.findModule(modulePath, alternativeModulePath, name, depth);
34+
this.findModule(resolvedDependencies, rootNodeModulesPath, modulePath, alternativeModulePath, name, depth);
4135
}
4236

43-
private findModule(modulePath: string, alternativeModulePath: string, name: string, depth: number) {
37+
private findModule(resolvedDependencies: IDependencyData[], rootNodeModulesPath: string, modulePath: string, rootModulesPath: string, name: string, depth: number): void {
4438
let exists = this.moduleExists(modulePath);
39+
let depthInNodeModules = depth + 1;
4540

46-
if (exists) {
47-
if (this.seen[modulePath]) {
48-
return;
49-
}
50-
51-
let dependency = this.addDependency(name, modulePath, depth + 1);
52-
this.readModuleDependencies(modulePath, depth + 1, dependency);
53-
} else {
54-
modulePath = alternativeModulePath; // /node_modules/<package>
41+
if (!exists) {
42+
modulePath = rootModulesPath; // /node_modules/<package>
5543
exists = this.moduleExists(modulePath);
5644

57-
if (!exists) {
58-
return;
59-
}
60-
61-
if (this.seen[modulePath]) {
62-
return;
63-
}
45+
depthInNodeModules = 0;
46+
}
6447

65-
let dependency = this.addDependency(name, modulePath, 0);
66-
this.readModuleDependencies(modulePath, 0, dependency);
48+
if (!exists || _.some(resolvedDependencies, deps => deps.directory === modulePath)) {
49+
return;
6750
}
6851

69-
this.seen[modulePath] = true;
52+
const dependency = this.getDependencyInfo(name, modulePath, depthInNodeModules);
53+
resolvedDependencies.push(dependency);
54+
55+
this.readModuleDependencies(resolvedDependencies, rootNodeModulesPath, modulePath, depthInNodeModules, dependency);
7056
}
7157

72-
private readModuleDependencies(modulePath: string, depth: number, currentModule: any): void {
73-
let packageJsonPath = path.join(modulePath, 'package.json');
74-
let packageJsonExists = fs.lstatSync(packageJsonPath).isFile();
58+
private readModuleDependencies(resolvedDependencies: IDependencyData[], rootNodeModulesPath: string, modulePath: string, depth: number, currentModule: IDependencyData): void {
59+
const packageJsonPath = path.join(modulePath, PACKAGE_JSON_FILE_NAME);
60+
const packageJsonExists = this.$fs.getLsStats(packageJsonPath).isFile();
7561

7662
if (packageJsonExists) {
7763
let packageJsonContents = this.$fs.readJson(packageJsonPath);
@@ -81,31 +67,31 @@ export class NodeModulesDependenciesBuilder implements INodeModulesDependenciesB
8167
currentModule.nativescript = packageJsonContents.nativescript;
8268
}
8369

84-
_.keys(packageJsonContents.dependencies).forEach((dependencyName) => {
85-
this.traverseDependency(dependencyName, modulePath, depth);
86-
});
70+
_.keys(packageJsonContents.dependencies)
71+
.forEach((dependencyName) => {
72+
this.traverseDependency(resolvedDependencies, rootNodeModulesPath, dependencyName, modulePath, depth);
73+
});
8774
}
8875
}
8976

90-
private addDependency(name: string, directory: string, depth: number): any {
91-
let dependency: any = {
77+
private getDependencyInfo(name: string, directory: string, depth: number): IDependencyData {
78+
const dependency: IDependencyData = {
9279
name,
9380
directory,
9481
depth
9582
};
9683

97-
this.resolvedDependencies.push(dependency);
98-
9984
return dependency;
10085
}
10186

10287
private moduleExists(modulePath: string): boolean {
10388
try {
104-
let exists = fs.lstatSync(modulePath);
105-
if (exists.isSymbolicLink()) {
106-
exists = fs.lstatSync(fs.realpathSync(modulePath));
89+
let modulePathLsStat = this.$fs.getLsStats(modulePath);
90+
if (modulePathLsStat.isSymbolicLink()) {
91+
modulePathLsStat = this.$fs.getLsStats(this.$fs.realpath(modulePath));
10792
}
108-
return exists.isDirectory();
93+
94+
return modulePathLsStat.isDirectory();
10995
} catch (e) {
11096
return false;
11197
}

lib/tools/node-modules/node-modules-dest-copy.ts

+7-7
Original file line numberDiff line numberDiff line change
@@ -61,18 +61,18 @@ export class NpmPluginPrepare {
6161
) {
6262
}
6363

64-
protected async beforePrepare(dependencies: IDictionary<IDependencyData>, platform: string, projectData: IProjectData): Promise<void> {
64+
protected async beforePrepare(dependencies: IDependencyData[], platform: string, projectData: IProjectData): Promise<void> {
6565
await this.$platformsData.getPlatformData(platform, projectData).platformProjectService.beforePrepareAllPlugins(projectData, dependencies);
6666
}
6767

68-
protected async afterPrepare(dependencies: IDictionary<IDependencyData>, platform: string, projectData: IProjectData): Promise<void> {
68+
protected async afterPrepare(dependencies: IDependencyData[], platform: string, projectData: IProjectData): Promise<void> {
6969
await this.$platformsData.getPlatformData(platform, projectData).platformProjectService.afterPrepareAllPlugins(projectData);
7070
this.writePreparedDependencyInfo(dependencies, platform, projectData);
7171
}
7272

73-
private writePreparedDependencyInfo(dependencies: IDictionary<IDependencyData>, platform: string, projectData: IProjectData): void {
73+
private writePreparedDependencyInfo(dependencies: IDependencyData[], platform: string, projectData: IProjectData): void {
7474
let prepareData: IDictionary<boolean> = {};
75-
_.values(dependencies).forEach(d => {
75+
_.each(dependencies, d => {
7676
prepareData[d.name] = true;
7777
});
7878
this.$fs.createDirectory(this.preparedPlatformsDir(platform, projectData));
@@ -101,18 +101,18 @@ export class NpmPluginPrepare {
101101
return this.$fs.readJson(this.preparedPlatformsFile(platform, projectData), "utf8");
102102
}
103103

104-
private allPrepared(dependencies: IDictionary<IDependencyData>, platform: string, projectData: IProjectData): boolean {
104+
private allPrepared(dependencies: IDependencyData[], platform: string, projectData: IProjectData): boolean {
105105
let result = true;
106106
const previouslyPrepared = this.getPreviouslyPreparedDependencies(platform, projectData);
107-
_.values(dependencies).forEach(d => {
107+
_.each(dependencies, d => {
108108
if (!previouslyPrepared[d.name]) {
109109
result = false;
110110
}
111111
});
112112
return result;
113113
}
114114

115-
public async preparePlugins(dependencies: IDictionary<IDependencyData>, platform: string, projectData: IProjectData): Promise<void> {
115+
public async preparePlugins(dependencies: IDependencyData[], platform: string, projectData: IProjectData): Promise<void> {
116116
if (_.isEmpty(dependencies) || this.allPrepared(dependencies, platform, projectData)) {
117117
return;
118118
}

test/npm-support.ts

+3-1
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import { XmlValidator } from "../lib/xml-validator";
2727
import { LockFile } from "../lib/lockfile";
2828
import ProjectChangesLib = require("../lib/services/project-changes-service");
2929
import { Messages } from "../lib/common/messages/messages";
30+
import { NodeModulesDependenciesBuilder } from "../lib/tools/node-modules/node-modules-dependencies-builder";
3031

3132
import path = require("path");
3233
import temp = require("temp");
@@ -81,9 +82,10 @@ function createTestInjector(): IInjector {
8182
testInjector.register("projectChangesService", ProjectChangesLib.ProjectChangesService);
8283
testInjector.register("emulatorPlatformService", stubs.EmulatorPlatformService);
8384
testInjector.register("analyticsService", {
84-
track: async () => undefined
85+
track: async (): Promise<any> => undefined
8586
});
8687
testInjector.register("messages", Messages);
88+
testInjector.register("nodeModulesDependenciesBuilder", NodeModulesDependenciesBuilder);
8789

8890
return testInjector;
8991
}

0 commit comments

Comments
 (0)