Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix getting production dependencies code #2789

Merged
merged 3 commits into from
May 11, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/bootstrap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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");
2 changes: 1 addition & 1 deletion lib/common
27 changes: 23 additions & 4 deletions lib/declarations.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -202,12 +202,31 @@ 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;
dependencies?: IStringDictionary;
devDependencies?: IStringDictionary;

/**
* 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;
}

interface IStaticConfig extends Config.IStaticConfig { }
Expand Down
2 changes: 1 addition & 1 deletion lib/definitions/platform.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ interface INodeModulesBuilder {
}

interface INodeModulesDependenciesBuilder {
getProductionDependencies(projectPath: string): void;
getProductionDependencies(projectPath: string): IDependencyData[];
}

interface IBuildInfo {
Expand Down
2 changes: 1 addition & 1 deletion lib/definitions/project.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion lib/services/android-project-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
7 changes: 3 additions & 4 deletions lib/services/livesync/livesync-service.ts
Original file line number Diff line number Diff line change
@@ -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");

Expand All @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down
7 changes: 3 additions & 4 deletions lib/tools/node-modules/node-modules-builder.ts
Original file line number Diff line number Diff line change
@@ -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> {
Expand All @@ -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, {
Expand Down
106 changes: 46 additions & 60 deletions lib/tools/node-modules/node-modules-dependencies-builder.ts
Original file line number Diff line number Diff line change
@@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wanted to share a personal opinion here. So, I think it's been a little bit more complicated than neccessary.

Why can't we combine this in one method and not have nested and hard to debug (for me) calls like:

---- many traverseDependency calls
-------- findModule
------------ readModuleDependencies
---------------- many traverse dependency calls

This nesting sounds too much to me. I'd suggest two approaches for simplification:

If we want to use recursion, why not rewrite it as:

  1. many traverseDependency(root_module, depth: 0)
    2.0 if (alreadyResolved(root_module)) return;
    2.1 let currentModule = findCurrentModule
    2.2 resolved[ currentModule ] = currentModule;
    2.3 for(dep in currentModule.dependencies) traverseDependency(dep, depth + 1)

This way we can remove the 3-4 nested methods.

We can use iteration instead

Another suggestion would be to just simulate a queue and turn this into a breadth-first-search, which I would find easier to debug, too. This way we will never care about the callstack anymore and we can debug it easier

  1. for(module in all_initial_modules) resolved[module] = module
  2. queue.push(all_initial_modules)
  3. while(queue.length > 0)
    2.1 let currentModule = queue.shift()
    2.2. for(dep in currentModule.dependencies)
    2.3. if (!alreadyResolved(dep)) {
    2.4. resolved[dep] = dep;
    2.5. queue.push(dep);

Thanks to your tests, we can refactor it at some point if we agree on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were discussing the same, but I didn't have time to implement it last week. As it looks like we all find the current code difficult to read, I've invested some time to implement your second suggestion.
Thanks for the idea.

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);
Expand All @@ -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;
}
Expand Down
18 changes: 9 additions & 9 deletions lib/tools/node-modules/node-modules-dest-copy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ export class TnsModulesCopy {
) {
}

public copyModules(dependencies: any[], platform: string): void {
public copyModules(dependencies: IDependencyData[], platform: string): void {
for (let entry in dependencies) {
let dependency = dependencies[entry];

Expand All @@ -34,7 +34,7 @@ export class TnsModulesCopy {
}
}

private copyDependencyDir(dependency: any): void {
private copyDependencyDir(dependency: IDependencyData): void {
if (dependency.depth === 0) {
let isScoped = dependency.name.indexOf("@") === 0;
let targetDir = this.outputRoot;
Expand All @@ -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));
Expand Down Expand Up @@ -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;
}
Expand Down
4 changes: 3 additions & 1 deletion test/npm-support.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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;
}
Expand Down
Loading