remove npm2 dependency #2209

Plamen5kov · 2016-11-09T08:51:12Z

What does this PR do?

PR removes the local npm2 dependency from CLI. Fix for: #1845

Description
Cache functionality is removed from lib/node-package-manager.ts and lib/npm-installation-manager. Other changes are made along the way to accommodate the lack of the cache add cache unpack functionality.

How did it work till now?

On npm install template: template is saved to tmp folder in some cases and to cache in others, and then are extracted to app folder. If Template is not default, the default template was downloaded because it's the only one that has App_Resources folder. The App_Resources folder is copied to new template.
On npm install core-framework (tns-ios, tns-android), tns-core-framework is saved to cache and extracted to platforms/ folder
On npm install node-inspector, package is saved and extracted to cache.
On tns platform update we always updated iOS

How will it work after this PR

On npm install, all packages are treated as normal node packages. That means all of them are installed in the node modules and in the case of the templates and the core-frameworks the packages are uninstalled after they are extracted to app/ and to platforms/ folders respectively. All templates will be expected to have an App_Resources folder with them.
We check the current iOS proj file and the new one and if they are different, there is a failure on update.
Currently the ios-node-inspector that is being installed through npm is in node_modules folder for each project. We might think of a way to make an exception for the inspector because of it's size.

The implementation works with npm cache internally.

Expected

Because not all templates have an App_Resource folder yet, there are tests that are expected to fail with: Project's app/App_Resources must contain Android and iOS directories.

Related PR

These PR add App_Resource folder to templates that don't have it. From now on all templates will have to carry with them an App_Resource folder.
https://github.com/NativeScript/template-hello-world-ts/pull/21
https://github.com/NativeScript/template-hello-world-ng/pull/34

ping: @NativeScript/android-runtime @rosen-vladimirov @hshristov @tzraikov @hdeshev @KristinaKoeva

hdeshev · 2016-11-09T11:18:29Z

lib/node-package-manager.ts

-				}
+			return diff;
+		} catch (err) {
+			if (err.code === "EPEERINVALID") {


Since we're not using npm as a library anymore, will we actually get an exception with an EPEERINVALID code from the child process? Seems like we should either delete this code or handle invalid peer dependencies in another way.

You're right I hadn't thought about that, will make adjustments
EDIT: @hdeshev after some research it turns out we throw exception on exec and this catch is executed, so we're safe

With new implementation we handle the EPEERINVALID error same as before.

hdeshev · 2016-11-09T11:23:51Z

lib/node-package-manager.ts

+			let subDirsBefore:Array<string> = this.$fs.exists(path.join(pathToSave, "node_modules")).wait() ? this.$fs.readDirectory(path.join(pathToSave, "node_modules")).wait() : [];
+
+			let flags = this.getFlagsString(config);
+			this.$childProcess.exec(`npm install ${packageName} ${flags}`, { cwd: pathToSave }).wait();


This exec invocation looks like it's eating the child process stdout. I think we should inherit stdio here so that npm-related errors are visible to the user. Hmm, yes, we need stdin too -- I've heard rumors on the internet there are packages that block the console asking for email addresses on installation.

@hdeshev inside childProcess.exec we do a throw which is caught by the catch, so we don't consume the error: https://github.com/telerik/mobile-cli-lib/blob/16a4c64e6f1ac2c58d21d92a5da11bc73854451c/child-process.ts#L14

In the new version i use $childProcess.spawnFromEvent(...) so we can handle the stdout and stderr to keep previous behavior.

hdeshev · 2016-11-09T11:37:30Z

lib/services/ios-project-service.ts

 		}).future<boolean>()();
 	}

+	// Is this used?


I suggest we kill this commented out code block..

hdeshev · 2016-11-09T11:39:37Z

test/project-templates-service.ts

-				assert.strictEqual(path.basename(actualPathToTemplate), expectedTemplatePath);
-				assert.strictEqual(isDeleteDirectoryCalledForNodeModulesDir, true, "When correct path is returned, template's node_modules directory should be deleted.");
-			});
+			//there is no tmp directory anymore


Delete commented-out code.

hdeshev · 2016-11-09T11:40:39Z

Squash those commits, please.

rosen-vladimirov · 2016-11-09T21:49:49Z

lib/node-package-manager.ts

-				}
-			});
-			return future;
+	public install(packageName: string, pathToSave: string, config?: any): any {


this method has async operation in it, so it should return IFuture. Also it's a good practice to avoid setting return type to any. As far as I can see, the return type here should be IFuture<string[]>

rosen-vladimirov · 2016-11-09T22:17:06Z

lib/node-package-manager.ts

-				return;
+			let flags = this.getFlagsString(config, true);
+			let params = ["install", packageName];
+			for(let index in flags) {


you can directly concat the params and flags arrays:

let params = [ "install", packageName ].concat(flags);

tried, didn't work :D

rosen-vladimirov · 2016-11-09T22:19:21Z

lib/node-package-manager.ts

-			if (this.$options.ignoreScripts) {
-				config = config || {};
-				config["ignore-scripts"] = true;
+			let res = this.$childProcess.spawnFromEvent("npm", params, "close", {}, { throwError: false }).wait();


I'm not entirely sure about this, but we've used to pass null for the fourth argument instead of empty object. This way the default childProcess options will be used.

rosen-vladimirov · 2016-11-09T22:30:10Z

lib/node-package-manager.ts

-				config["ignore-scripts"] = true;
+			let res = this.$childProcess.spawnFromEvent("npm", params, "close", {}, { throwError: false }).wait();
+			if(res.stderr) {
+				throw {stderr: res.stderr};


this way the error will not have message. Most of the cosumers which print errors use the message property of the error. You can use:

throw new Error(res.stderr);

and later you can check if err.message.indexOf("EPEERINVALID") !== -1

rosen-vladimirov · 2016-11-09T22:32:47Z

lib/node-package-manager.ts

+			 * 	The solution is to traverse node_modules before and after install and get the name of the installed package,
+			 * 	even if it's installed through local path or URL. If command installes more than one package, all package names are returned.
+			 */
+			let diff = subDirsAfter.filter(x => subDirsBefore.indexOf(x) === -1);


In case I want to install a single package, via .tgz, but it has dependencies, npm 3 will install all of them on the root level and current code will return list of all of them. But the method should have installed a single package and return its name. Is this behavior expected?

@rosen-vladimirov, yes this behavior is expected

rosen-vladimirov · 2016-11-10T06:51:46Z

lib/services/platform-service.ts

 	}

+	/**
+	 * Reads package.json/nativescript/tns-(android/ios)/template if any and installes it.
+	 * We need to remove this, because it may override the template already present in app if someone decides to 


platform template is the template used for platforms/<platform>/ directory. It cannot override anything in app dir.

rosen-vladimirov · 2016-11-10T06:57:53Z

lib/services/platform-service.ts

 				}
 			} else {
-				this.updatePlatformCore(platformData, currentVersion, newVersion, canUpdate).wait();
+				this.$errors.fail("Native Platfrom cannot be updated.");


Platfrom -> Platform
Using $errors.fail will print command's help after the error, is this required here or failWithoutHelp is more appropriate?

rosen-vladimirov · 2016-11-10T07:16:31Z

lib/services/plugins-service.ts

+			//npmInstallResultStream is not very usefull (doesn't contain all the package information), so we need to find the real one inside node_modules
+			let foundValidPlugin = false;
+			let realNpmPackageJson:any;
+			for(let index in installedModuleNames) {


you can use of instead of in which will give you the name directly:

for(let name of installedModuleNames) { ...

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of

rosen-vladimirov · 2016-11-10T07:17:42Z

lib/services/plugins-service.ts

+			for(let index in installedModuleNames) {
+				let name = installedModuleNames[index];
+				let pathToRealNpmPackageJson = `${this.$projectData.projectDir}/node_modules/${name}/package.json`;
+				realNpmPackageJson = require(pathToRealNpmPackageJson);


it's safer to use this.$fs.readJson(pathToRealNpmPackageJson).wait() as require caches results and also it does not work in case the file has BOM header

rosen-vladimirov · 2016-11-10T07:33:06Z

lib/definitions/project.d.ts

 	/**
 	* Provides a platform specific update logic for the specified runtime versions.
 	* @return true in cases when the update procedure should continue.
 	*/
-	updatePlatform(currentVersion: string, newVersion: string, canUpdate: boolean, addPlatform?: Function, removePlatform?: (platforms: string[]) => IFuture<void>): IFuture<boolean>;
+	// updatePlatform(currentVersion: string, newVersion: string, canUpdate: boolean, addPlatform?: Function, removePlatform?: (platforms: string[]) => IFuture<void>): IFuture<boolean>;


In case the method is commented, can you please delete it. Also the implementation should be removed.

rosen-vladimirov · 2016-11-10T19:36:07Z

lib/node-package-manager.ts

 	}

 	public search(filter: string[], silent: boolean): IFuture<any> {
 		let args = (<any[]>([filter] || [])).concat(silent);
-		return this.loadAndExecute("search", args);
+		return this.$childProcess.exec(`npm search ${args}`);


As args is array, it's toString representation will be the elements, separated by ,. For example in case the args is array: [ "a", "b", "c" ], the execed process will be:

npm search a,b,c

In fact we would like the command to be:

npm search a b c

(note the results of the two search commands are totally different).

So I would advise you to use:

return this.$childProcess.exec(`npm search ${args.join(" ")}`);

rosen-vladimirov · 2016-11-10T19:37:45Z

lib/node-package-manager.ts

@@ -81,66 +88,39 @@ export class NodePackageManager implements INodePackageManager {
 	}

 	public uninstall(packageName: string, config?: any, path?: string): IFuture<any> {
-		return this.loadAndExecute("uninstall", [[packageName]], { config, path });
+		let flags = this.getFlagsString(config, false);
+		return this.$childProcess.exec(`npm uninstall ${packageName} ${flags}`, { cwd: path });
 	}

 	public search(filter: string[], silent: boolean): IFuture<any> {
 		let args = (<any[]>([filter] || [])).concat(silent);


silent is boolean, so the args passed to npm search will contain the value true or false. Maybe you should add --silent only in case the silent argument is true.

if silent is passed getFlagsString function will add it as a flag, so this case is covered.

rosen-vladimirov · 2016-11-10T19:45:04Z

lib/services/plugins-service.ts

+			let foundValidPlugin = false;
+			let realNpmPackageJson:any;
+			for(let name of installedModuleNames) {
+				let pathToRealNpmPackageJson = `${this.$projectData.projectDir}/node_modules/${name}/package.json`;


you should use path.join when constructing file path instead of directly adding / as it may no work on all OSes

rosen-vladimirov · 2016-11-10T19:47:04Z

lib/services/plugins-service.ts

 			}

-			return this.parseNpmCommandResult(result);
+			return this.parseNpmCommandResult(npmCommandArguments);
 		}).future<string>()();
 	}

 	private parseNpmCommandResult(npmCommandResult: string): string {  // [[name@version, node_modules/name]]


I believe the comment is no longer correct, can you please remove it.

rosen-vladimirov · 2016-11-10T20:29:31Z

lib/node-package-manager.ts

+				 * 	The solution is to traverse node_modules before and after install and get the name of the installed package,
+				 * 	even if it's installed through local path or URL. If command installes more than one package, all package names are returned.
+				 */
+				let diff = dependenciesAfter.filter(x => dependenciesBefore.indexOf(x) === -1);


I've found a case where this approach will not work. Consider the case where a plugin is already installed in package.json, but user wants to update it's version (or install it from .tgz this time). In this case the difference will be empty array and the plugin will not be installed. I would suggest to compare both the name of the plugin and the version. For example this code could do the job:

let diff = _(jsonContentAfter.dependencies) .omitBy((val: string, key: string) => jsonContentBefore && jsonContentBefore.dependencies[key] && jsonContentBefore.dependencies[key] === val) .keys() .value();

rosen-vladimirov · 2016-11-14T21:57:25Z

lib/node-package-manager.ts

@@ -48,30 +24,54 @@ export class NodePackageManager implements INodePackageManager {
 			}

 			try {
-				return this.loadAndExecute("install", [pathToSave, packageName], { config: config }).wait();
+				let jsonContentBefore = this.$fs.readJson(path.join(pathToSave, "package.json")).wait();
+				let dependenciesBefore: Array<string> = [];


you do not have to iterate over the dependencies in order to get the keys. You can use lodash here:

let dependenciesBefore = _.keys(jsonContentBefore.dependencies);

Lodash's keys method will return empty array in case there's no dependencies property in jsonContentBefore

rosen-vladimirov · 2016-11-14T21:57:59Z

lib/node-package-manager.ts

+				this.$childProcess.spawnFromEvent("npm", params, "close", { cwd: pathToSave }).wait();
+
+				let jsonContentAfter = this.$fs.readJson(path.join(pathToSave, "package.json")).wait();
+				let dependenciesAfter: Array<string> = [];


same as above - you can skip the if and use _.keys(jsonContentAfter.dependencies);

rosen-vladimirov · 2016-11-14T21:59:43Z

lib/node-package-manager.ts

 			} catch (err) {
-				if (err.code === "EPEERINVALID") {
+				if (err.message && err.message.indexOf("EPEERINVALID") !== -1) {


In case npm install fails with EPEERINVALID error, the method will show it as warning and will skip all of the checks regarding jsonContentBefore and jsonContentAfter. Also the method will not return anything. Is this expected?

It's not, I'll fix it

rosen-vladimirov · 2016-11-14T22:10:25Z

lib/npm-installation-manager.ts

-				// this package has not been published to npmjs.org yet - perhaps manually added via --framework-path
-				this.$logger.trace(`Checking shasum of package ${packageName}@${version}: skipped because the package was not found in npmjs.org`);
-				return true;
+			if(version === constants.PackageVersion.NEXT) {


why do you need special code for next version. In case we decide to use separate tag (for example nextNext) we shouldn't add it here. Npm supports installing of package directly by tag, for example npm install nativescript@next will work as expected. Is there a particular reason to have special methods for next and latest tags?

I've missed removing, this if, i was testing something out, and I've seem to forgot to remove it. The idea behind the next and last tags is readability only.

rosen-vladimirov · 2016-11-14T22:11:16Z

lib/npm-installation-manager.ts

 			}
+			let installedModuleNames = this.npmInstall(packageName, pathToSave, version, dependencyType).wait();
+			let installedPackageName =  installedModuleNames[0];


Why [0] - is it possible to return more than one packages? In case yes - why do we get only the first one. In case not - why does the method return array?

On all the places we use node-package-manager, we pass a single package to be installed, and expect a single package to be installed. My thinking was for a later refactoring, where we could be able to use the node-package-manager everywhere or removing it completely and using npm service instead. That means it could install one or multiple packages, hence saving the array result.

rosen-vladimirov · 2016-11-14T22:22:35Z

lib/services/platform-service.ts

 				}
 			} else {
-				this.updatePlatformCore(platformData, currentVersion, newVersion, canUpdate).wait();
+				this.$errors.failWithoutHelp("Native Platfrom cannot be updated.");


Platfrom -> Platform

rosen-vladimirov · 2016-11-14T22:34:23Z

lib/node-package-manager.ts

+					params.push(packageName); //because npm install ${pwd} on mac tries to install itself as a dependency (windows and linux have no such issues)
+				}
+				params = params.concat(flags);
+				this.$childProcess.spawnFromEvent("npm", params, "close", { cwd: pathToSave }).wait();


How does this code work on Windows? On Mac and Linux the executable name is npm, but on Windows it's npm.cmd

As I'm not calling a direct path to executable, I expect npm to be found in path.

There's no npm executable in PATH on Windows. There's npm.cmd

To be more precise in the explanation:

on Windows, the default command prompt is able to resolve npm to npm.cmd when you execute npm install

childProcess.spawn method does not launch new shell, it just starts executable in PATH (or passed with full path)

In the current case, childProcess is trying to start a new process called npm but it cannot be resolved as the real name of the executable on Windows is npm.cmd.
We have such code, you can take a look at it here

I understand, I'll fix it, thank you.

Plamen5kov · 2016-11-15T15:16:48Z

run CI

Plamen5kov · 2016-11-16T11:58:16Z

@hdeshev @rosen-vladimirov, 5 tests concerning App_Resources folder are commented out so CI runs ok, but if you have any last notes for the PR, I would be happy to resolve them.

NativeScript/nativescript-cli#2209

fixed broken test … test was broken because of npm3 node_modules structure test logic is same

…npm2_dependency" This reverts commit 40566ce, reversing changes made to 0be61c8.

…/remove_npm2_dependency"" This reverts commit a60eabd.

Plamen5kov force-pushed the plamen5kov/remove_npm2_dependency branch 2 times, most recently from 23c9293 to 6c50fb5 Compare November 9, 2016 08:58

hdeshev suggested changes Nov 9, 2016

View reviewed changes

Plamen5kov force-pushed the plamen5kov/remove_npm2_dependency branch 2 times, most recently from 67619c9 to bc808aa Compare November 10, 2016 07:02

rosen-vladimirov suggested changes Nov 10, 2016

View reviewed changes

Plamen5kov force-pushed the plamen5kov/remove_npm2_dependency branch 2 times, most recently from dd9e8dc to e777918 Compare November 14, 2016 07:03

Plamen5kov mentioned this pull request Nov 14, 2016

tns platform add --framework-path should support urls #2101

Closed

rosen-vladimirov suggested changes Nov 14, 2016

View reviewed changes

Plamen5kov force-pushed the plamen5kov/remove_npm2_dependency branch from 6b73294 to 92de5c7 Compare November 15, 2016 09:29

Plamen5kov force-pushed the plamen5kov/remove_npm2_dependency branch from 10484fd to e384c4b Compare November 16, 2016 11:49

vchimev pushed a commit to NativeScript/nativescript-cli-tests that referenced this pull request Nov 16, 2016

Add create project test

e18aedd

NativeScript/nativescript-cli#2209

hdeshev approved these changes Nov 16, 2016

View reviewed changes

Plamen5kov added 6 commits November 16, 2016 17:31

remove npm2 dependency + fixes after reviews

ef2fcbc

always save to dependencies on platform add

f2b53a4

comment out tests, so CI runs

8d0fb1e

fixed broken test

3832925

fixed broken test … test was broken because of npm3 node_modules structure test logic is same

updated submodule

5763a53

fix ios-debug after merge

fd15624

Plamen5kov force-pushed the plamen5kov/remove_npm2_dependency branch from e384c4b to fd15624 Compare November 16, 2016 15:41

rosen-vladimirov approved these changes Nov 17, 2016

View reviewed changes

Plamen5kov merged commit 40566ce into master Nov 17, 2016

Plamen5kov added a commit that referenced this pull request Nov 17, 2016

Revert "Merge pull request #2209 from NativeScript/plamen5kov/remove_…

a60eabd

…npm2_dependency" This reverts commit 40566ce, reversing changes made to 0be61c8.

Plamen5kov added a commit that referenced this pull request Nov 24, 2016

Revert "Revert "Merge pull request #2209 from NativeScript/plamen5kov…

89b27ac

…/remove_npm2_dependency"" This reverts commit a60eabd.

remove npm2 dependency #2209

remove npm2 dependency #2209

Conversation

Plamen5kov commented Nov 9, 2016 • edited Loading

What does this PR do?

How did it work till now?

How will it work after this PR

Expected

Related PR

Choose a reason for hiding this comment

Plamen5kov Nov 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Plamen5kov Nov 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hdeshev commented Nov 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Plamen5kov Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Plamen5kov commented Nov 15, 2016

Plamen5kov commented Nov 16, 2016

Plamen5kov commented Nov 9, 2016 •

edited

Loading

Plamen5kov Nov 9, 2016 •

edited

Loading

Plamen5kov Nov 9, 2016 •

edited

Loading

Plamen5kov Nov 15, 2016 •

edited

Loading