Skip to content

Copy node_modules to platform on prepare (fix Node6/npm 3.x bug) #2152

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Oct 26, 2016
Merged

Copy node_modules to platform on prepare (fix Node6/npm 3.x bug) #2152

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fdec58f
run npm list --prod command and get all dependency names
Plamen5kov Oct 17, 2016
77f86a3
add npm-independent traversal to get project's production dependencies
petekanev Oct 20, 2016
7b10417
separate logic for determining production dependencies in a class
petekanev Oct 24, 2016
4c483d0
removed unnecessary dependency
Plamen5kov Oct 25, 2016
b569b6e
remove node_modules of tns-core-modules if present when copied into t…
petekanev Oct 25, 2016
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
160 changes: 149 additions & 11 deletions lib/tools/node-modules/node-modules-builder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@ import * as fs from "fs";
import * as path from "path";
import * as shelljs from "shelljs";
import Future = require("fibers/future");
import {NpmDependencyResolver, TnsModulesCopy, NpmPluginPrepare} from "./node-modules-dest-copy";
import { TnsModulesCopy, NpmPluginPrepare } from "./node-modules-dest-copy";
import * as fiberBootstrap from "../../common/fiber-bootstrap";
import {sleep} from "../../../lib/common/helpers";
import { sleep } from "../../../lib/common/helpers";

let glob = require("glob");

export class NodeModulesBuilder implements INodeModulesBuilder {
constructor(
constructor(private $childProcess: IChildProcess,
private $fs: IFileSystem,
private $projectData: IProjectData,
private $projectDataService: IProjectDataService,
Expand All @@ -37,7 +37,7 @@ export class NodeModulesBuilder implements INodeModulesBuilder {
}, (er: Error, files: string[]) => {
fiberBootstrap.run(() => {

while(this.$lockfile.check().wait()) {
while (this.$lockfile.check().wait()) {
sleep(10);
}

Expand Down Expand Up @@ -85,7 +85,7 @@ export class NodeModulesBuilder implements INodeModulesBuilder {
let intervalId = setInterval(() => {
fiberBootstrap.run(() => {
if (!this.$lockfile.check().wait() || future.isResolved()) {
if(!future.isResolved()) {
if (!future.isResolved()) {
future.return();
}
clearInterval(intervalId);
Expand Down Expand Up @@ -133,27 +133,165 @@ export class NodeModulesBuilder implements INodeModulesBuilder {
// Force copying if the destination doesn't exist.
lastModifiedTime = null;
}
let nodeModules = this.getChangedNodeModules(absoluteOutputPath, platform, lastModifiedTime).wait();

const resolver = new NpmDependencyResolver(this.$projectData.projectDir);
const resolvedDependencies = resolver.resolveDependencies(_.keys(nodeModules), platform);
let productionDependencies = this.getProductionDependencies(this.$projectData.projectDir);

// console.log(productionDependencies);

// TODO: Pip3r4o - result is not used currently
// let nodeModules = this.getChangedNodeModules(absoluteOutputPath, platform, lastModifiedTime).wait();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get rid of commented out code?


if (!this.$options.bundle) {
const tnsModulesCopy = this.$injector.resolve(TnsModulesCopy, {
outputRoot: absoluteOutputPath
});
tnsModulesCopy.copyModules(resolvedDependencies, platform);
tnsModulesCopy.copyModules(productionDependencies, platform);
} else {
this.cleanNodeModules(absoluteOutputPath, platform);
}

const npmPluginPrepare = this.$injector.resolve(NpmPluginPrepare, {});
npmPluginPrepare.preparePlugins(resolvedDependencies, platform);
npmPluginPrepare.preparePlugins(productionDependencies, platform);
}).future<void>()();
}

public getProductionDependencies(projectPath: string) {
var deps: any = [];
var seen: any = {};

var pJson = path.join(projectPath, "package.json");
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, avoid weird abbreviations like pJson. How about packageJsonPath?

var nodeModulesDir = path.join(projectPath, "node_modules");

var content = require(pJson);

Object.keys(content.dependencies).forEach((key) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

if there's no dependencies section in the package.json, this code will fail. I would use lodash's _.each:

_.keys(content.dependencies)
   .forEach(key => { 
    ....
    });

var depth = 0;
var directory = path.join(nodeModulesDir, key);

// find and traverse child with name `key`, parent's directory -> dep.directory
traverseChild(key, directory, depth);
});

return filterUniqueDirectories(deps);

function traverseChild(name: string, currentModulePath: string, depth: number) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Those inner functions are very confusing and unreadable. Consider moving them to methods of this class or an entirely new class.

// check if key appears in a scoped module dependency
var isScoped = name.indexOf('@') === 0;

if (!isScoped) {
// 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
var modulePath = path.join(currentModulePath, "node_modules", name); // /node_modules/parent/node_modules/<package>
var exists = ensureModuleExists(modulePath);

if (exists) {
var dep = addDependency(deps, name, modulePath, depth + 1);

traverseModule(modulePath, depth + 1, dep);
} else {
modulePath = path.join(nodeModulesDir, name); // /node_modules/<package>
exists = ensureModuleExists(modulePath);

if(!exists) {
return;
}

var dep = addDependency(deps, name, modulePath, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, please avoid abbreviations: dep


traverseModule(modulePath, 0, dep);
}

}
// module is scoped
else {
Copy link
Contributor

Choose a reason for hiding this comment

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

} else {

var scopeSeparatorIndex = name.indexOf('/');
var scope = name.substring(0, scopeSeparatorIndex);
var moduleName = name.substring(scopeSeparatorIndex + 1, name.length);
var scopedModulePath = path.join(nodeModulesDir, scope, moduleName);

var exists = ensureModuleExists(scopedModulePath);

if (exists) {
var dep = addDependency(deps, name, scopedModulePath, 0);
traverseModule(scopedModulePath, depth, dep);
}
else {
Copy link
Contributor

Choose a reason for hiding this comment

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

} else {

scopedModulePath = path.join(currentModulePath, "node_modules", scope, moduleName);

exists = ensureModuleExists(scopedModulePath);

if (!exists) {
return;
}

var dep = addDependency(deps, name, scopedModulePath, depth + 1);
traverseModule(scopedModulePath, depth + 1, dep);
}
}

function traverseModule(modulePath: string, depth: number, currentDependency: any) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the difference when traversing a "child" and a "module". I think one of these functions needs a better name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also please add return type to the function

var packageJsonPath = path.join(modulePath, 'package.json');
var packageJsonExists = fs.lstatSync(packageJsonPath).isFile();

if (packageJsonExists) {
var packageJsonContents = require(packageJsonPath);

if (!!packageJsonContents.nativescript) {
// add `nativescript` property, necessary for resolving plugins
currentDependency.nativescript = packageJsonContents.nativescript;
}

if (packageJsonContents.dependencies) {
Object.keys(packageJsonContents.dependencies).forEach((key) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above - it's safer to use:

_.keys(packageJsonContent.dependencies).forEach(...


traverseChild(key, modulePath, depth);
});
}
}
}

function addDependency(deps: any[], name: string, directory: string, depth: number) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this function needs return type

var dep: any = {};
dep.name = name;
dep.directory = directory;
dep.depth = depth;

deps.push(dep);

return dep;
}

function ensureModuleExists(modulePath: string): boolean {
try {
var exists = fs.lstatSync(modulePath);
return exists.isDirectory();
} catch (e) {
return false;
}
}
}

function filterUniqueDirectories(dependencies: any) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use a map keyed by the dependency directory here (simulating a set collection) instead of doing linear traversals all the time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted. I used a string map initially to check if the plugin's already present in the list, but that approach omitted corner cases where 2 plugins with different versions were present. I'll revisit it again

var unique: any = [];
var distinct: any = [];
for (var i in dependencies) {
var dep = dependencies[i];
if (distinct.indexOf(dep.directory) > -1) {
continue;
}

distinct.push(dep.directory);
unique.push(dep);
}

return unique;
}
}

public cleanNodeModules(absoluteOutputPath: string, platform: string): void {
shelljs.rm("-rf", absoluteOutputPath);
}
}
}

$injector.register("nodeModulesBuilder", NodeModulesBuilder);
113 changes: 18 additions & 95 deletions lib/tools/node-modules/node-modules-dest-copy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,96 +10,17 @@ export interface ILocalDependencyData extends IDependencyData {
directory: string;
}

export class NpmDependencyResolver {
constructor(
private projectDir: string
) {
}

private getDevDependencies(projectDir: string): IDictionary<any> {
let projectFilePath = path.join(projectDir, constants.PACKAGE_JSON_FILE_NAME);
let projectFileContent = require(projectFilePath);
return projectFileContent.devDependencies || {};
}

public resolveDependencies(changedDirectories: string[], platform: string): IDictionary<ILocalDependencyData> {
const devDependencies = this.getDevDependencies(this.projectDir);
const dependencies: IDictionary<ILocalDependencyData> = Object.create(null);

_.each(changedDirectories, changedDirectoryAbsolutePath => {
if (!devDependencies[path.basename(changedDirectoryAbsolutePath)]) {
let pathToPackageJson = path.join(changedDirectoryAbsolutePath, constants.PACKAGE_JSON_FILE_NAME);
let packageJsonFiles = fs.existsSync(pathToPackageJson) ? [pathToPackageJson] : [];
let nodeModulesFolderPath = path.join(changedDirectoryAbsolutePath, constants.NODE_MODULES_FOLDER_NAME);
packageJsonFiles = packageJsonFiles.concat(this.enumeratePackageJsonFilesSync(nodeModulesFolderPath));

_.each(packageJsonFiles, packageJsonFilePath => {
let fileContent = require(packageJsonFilePath);

if (!devDependencies[fileContent.name] && fileContent.name && fileContent.version) { // Don't flatten dev dependencies and flatten only dependencies with valid package.json
let currentDependency: ILocalDependencyData = {
name: fileContent.name,
version: fileContent.version,
directory: path.dirname(packageJsonFilePath),
nativescript: fileContent.nativescript
};

let addedDependency = dependencies[currentDependency.name];
if (addedDependency) {
if (semver.gt(currentDependency.version, addedDependency.version)) {
let currentDependencyMajorVersion = semver.major(currentDependency.version);
let addedDependencyMajorVersion = semver.major(addedDependency.version);

let message = `The dependency located at ${addedDependency.directory} with version ${addedDependency.version} will be replaced with dependency located at ${currentDependency.directory} with version ${currentDependency.version}`;
let logger = $injector.resolve("$logger");
currentDependencyMajorVersion === addedDependencyMajorVersion ? logger.out(message) : logger.warn(message);

dependencies[currentDependency.name] = currentDependency;
}
} else {
dependencies[currentDependency.name] = currentDependency;
}
}
});
}
});
return dependencies;
}

private enumeratePackageJsonFilesSync(nodeModulesDirectoryPath: string, foundFiles?: string[]): string[] {
foundFiles = foundFiles || [];
if (fs.existsSync(nodeModulesDirectoryPath)) {
let contents = fs.readdirSync(nodeModulesDirectoryPath);
for (let i = 0; i < contents.length; ++i) {
let moduleName = contents[i];
let moduleDirectoryInNodeModules = path.join(nodeModulesDirectoryPath, moduleName);
let packageJsonFilePath = path.join(moduleDirectoryInNodeModules, constants.PACKAGE_JSON_FILE_NAME);
if (fs.existsSync(packageJsonFilePath)) {
foundFiles.push(packageJsonFilePath);
}

let directoryPath = path.join(moduleDirectoryInNodeModules, constants.NODE_MODULES_FOLDER_NAME);
if (fs.existsSync(directoryPath)) {
this.enumeratePackageJsonFilesSync(directoryPath, foundFiles);
} else if (fs.statSync(moduleDirectoryInNodeModules).isDirectory()) {
// Scoped modules (e.g. @angular) are grouped in a subfolder and we need to enumerate them too.
this.enumeratePackageJsonFilesSync(moduleDirectoryInNodeModules, foundFiles);
}
}
}
return foundFiles;
}
}

export class TnsModulesCopy {
constructor(
private outputRoot: string,
private $fs: IFileSystem
) {
}

public copyModules(dependencies: IDictionary<ILocalDependencyData>, platform: string): void {
_.each(dependencies, dependency => {
public copyModules(dependencies: any[], platform: string): void {
for (var entry in dependencies) {
Copy link
Contributor

Choose a reason for hiding this comment

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

var -> let

var dependency = dependencies[entry];
Copy link
Contributor

Choose a reason for hiding this comment

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

var -> let


this.copyDependencyDir(dependency);

if (dependency.name === constants.TNS_CORE_MODULES_NAME) {
Expand All @@ -110,22 +31,24 @@ export class TnsModulesCopy {
let deleteFilesFutures = allFiles.filter(file => minimatch(file, "**/*.ts", { nocase: true })).map(file => this.$fs.deleteFile(file));
Future.wait(deleteFilesFutures);

shelljs.cp("-Rf", path.join(tnsCoreModulesResourcePath, "*"), this.outputRoot);
this.$fs.deleteDirectory(tnsCoreModulesResourcePath).wait();
shelljs.rm("-rf", path.join(this.outputRoot, dependency.name, "node_modules"));
}
});
}
}

private copyDependencyDir(dependency: any): void {
let dependencyDir = path.dirname(dependency.name || "");
let insideNpmScope = /^@/.test(dependencyDir);
let targetDir = this.outputRoot;
if (insideNpmScope) {
targetDir = path.join(this.outputRoot, dependencyDir);
private copyDependencyDir(dependency: any) {
if (dependency.depth === 0) {
let isScoped = dependency.name.indexOf("@") === 0;
let targetDir = this.outputRoot;

if (isScoped) {
targetDir = path.join(this.outputRoot, dependency.name.substring(0, dependency.name.indexOf("/")));
}

shelljs.mkdir("-p", targetDir);
shelljs.cp("-Rf", dependency.directory, targetDir);
// console.log("Copied dependency: " + dependency.name);
Copy link
Contributor

Choose a reason for hiding this comment

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

can you remove the commented line or add it as log trace message:

this.$logger.trace(`Copied dependency: ${dependency.name}`);

}
shelljs.mkdir("-p", targetDir);
shelljs.cp("-Rf", dependency.directory, targetDir);
shelljs.rm("-rf", path.join(targetDir, dependency.name, "node_modules"));
}
}

Expand Down