Use tns_modules from npm #805

e2l3n · 2015-08-20T18:09:50Z

Use npm to fetch tns-core-modules package.
Create tns_modules folder from tns-core-modules package.
See issue #390

ns-bot · 2015-08-20T18:09:54Z

Can one of the admins verify this patch?

ligaz · 2015-08-20T18:34:12Z

lib/definitions/project.d.ts

@@ -1,3 +1,8 @@
+interface ITNSModulesService {
+	tnsModulesInstallationPath(projectRoot: string): IFuture<string>;
+	NPM_TNS_CORE_MODULES_NAME: string;


The convention for property names is to use camelCase.

teobugslayer · 2015-08-20T18:37:47Z

The build fails executing unit tests with Error: unable to resolve tnsModulesService. Can you please verify that unit tests work?

teobugslayer · 2015-08-20T18:40:35Z

lib/services/tns-modules-service.ts

+///<reference path="../.d.ts"/>
+"use strict";
+
+import util = require("util");


Consider using ES2015 style imports where possible

ligaz · 2015-08-20T18:40:56Z

I'm not sure that the currently implemented workflow is the right one. The process should be like:

create new project from template
check if the template has tns_modules as dependencies and if not add them in the package.json
npm install

teobugslayer · 2015-08-20T18:41:07Z

lib/services/tns-modules-service.ts

+import path = require("path");
+import shell = require("shelljs");
+import npm = require("npm");
+var helpers = require("../common/helpers");


You can import here. By the way, are these imports used? If not, you can remove them.

teobugslayer · 2015-08-20T18:46:36Z

Consider writing unit tests for TNSModulesService.

Fatme · 2015-08-21T07:21:13Z

lib/services/project-service.ts

+            }
+            else {
+                let coreModulesName = this.$tnsModulesService.npmTnsCoreModulesName;
+                tnsModulesVersion = this.$fs.readJson(path.join(appPath, constants.PACKAGE_JSON_FILE_NAME)).wait()[coreModulesName]["version"];


Consider using $projectDataService instead readJson

ligaz · 2015-08-21T13:50:36Z

lib/services/project-service.ts

@@ -111,6 +113,9 @@ export class ProjectService implements IProjectService {

 			this.$projectDataService.initialize(projectDir);
 			this.$projectDataService.setValue("id", projectId).wait();
+			//Start child process because --save --save-exact option is necessary when installing tns-core-modules
+			//in order to create "dependencies" key in package.json. 
+			this.$childProcess.exec("npm install " + constants.TNS_CORE_MODULES_NAME + " --save --save-exact", { cwd: projectDir }).wait();


Should it be better to use INodePackageManager and expose this functionality there?

e2l3n · 2015-08-21T16:13:37Z

I will squash all commits right before merging this pull request.

Fatme · 2015-08-24T06:28:12Z

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

@@ -75,6 +75,11 @@ export class DestCopy implements IBroccoliPlugin {
 		if(isPlugin) {
 			this.$pluginsService.prepare(dependency).wait();
 		}
+
+		if (dependency.name == constants.TNS_CORE_MODULES_NAME) {


Use === instead ==

Fatme · 2015-08-25T07:54:11Z

lib/services/project-service.ts

+			let tnsModulesVersion: string = this.$options.tnsModulesVersion;
+			let packageName: string  = constants.TNS_CORE_MODULES_NAME;
+			if (tnsModulesVersion) {
+				packageName = packageName.concat("@" + tnsModulesVersion);


You can use packageName = ${packageName}@${tnsModulesVersion}

teobugslayer · 2015-08-25T12:40:36Z

lib/services/platform-service.ts

+				this.$broccoliBuilder.prepareNodeModules(tnsModulesDestinationPath, this.$projectData.projectDir, platform, lastModifiedTime).wait();
+			} catch(error) {
+				this.$logger.debug(error);
+				this.$logger.error("Processing tns core modules failed.");


This is user visible error. Consider at least showing the users the actual error. This way, they have a chance to understand what went wrong.

The error might be produced while processing some other module or plugin. It is not obligatory to be produced while "Processing tns core modules"

teobugslayer · 2015-08-25T12:42:38Z

👍 after addressing my small remark and a green build.

Fatme · 2015-08-25T12:54:01Z

lib/services/project-service.ts

@@ -111,6 +112,14 @@ export class ProjectService implements IProjectService {

 			this.$projectDataService.initialize(projectDir);
 			this.$projectDataService.setValue("id", projectId).wait();
+
+			let tnsModulesVersion: string = this.$options.tnsModulesVersion;
+			let packageName: string  = constants.TNS_CORE_MODULES_NAME;


You do not need to specify the variable type.

Use tns_modules from npm

ligaz reviewed Aug 20, 2015
View reviewed changes

teobugslayer reviewed Aug 20, 2015
View reviewed changes

e2l3n force-pushed the tpopov/tns-core-modules branch 3 times, most recently from abea2b7 to 3a7b69f Compare August 20, 2015 19:22

Fatme reviewed Aug 21, 2015
View reviewed changes

e2l3n force-pushed the tpopov/tns-core-modules branch from bcaba8b to 6215768 Compare August 21, 2015 13:41

ligaz reviewed Aug 21, 2015
View reviewed changes

e2l3n force-pushed the tpopov/tns-core-modules branch from 13761a1 to f768c15 Compare August 21, 2015 16:06

Fatme reviewed Aug 24, 2015
View reviewed changes

e2l3n force-pushed the tpopov/tns-core-modules branch from 34067fe to 1f1543d Compare August 24, 2015 19:41

Fatme reviewed Aug 25, 2015
View reviewed changes

e2l3n force-pushed the tpopov/tns-core-modules branch from c028a0e to 2a68d08 Compare August 25, 2015 11:47

e2l3n force-pushed the tpopov/tns-core-modules branch from 2a68d08 to 6d56327 Compare August 25, 2015 11:52

teobugslayer reviewed Aug 25, 2015
View reviewed changes

Fatme reviewed Aug 25, 2015
View reviewed changes

e2l3n force-pushed the tpopov/tns-core-modules branch from 6d56327 to d906519 Compare August 25, 2015 13:46

Use npm to fetch tns-core-modules package.

8f9283c

e2l3n force-pushed the tpopov/tns-core-modules branch from d906519 to 8f9283c Compare August 25, 2015 13:52

e2l3n added a commit that referenced this pull request Aug 25, 2015

Merge pull request #805 from e2l3n/tpopov/tns-core-modules

e57fa5f

Use tns_modules from npm

e2l3n merged commit e57fa5f into NativeScript:master Aug 25, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use tns_modules from npm #805

Use tns_modules from npm #805

e2l3n commented Aug 20, 2015

ns-bot commented Aug 20, 2015

ligaz Aug 20, 2015

teobugslayer commented Aug 20, 2015

teobugslayer Aug 20, 2015

ligaz commented Aug 20, 2015

teobugslayer Aug 20, 2015

teobugslayer commented Aug 20, 2015

Fatme Aug 21, 2015

ligaz Aug 21, 2015

e2l3n commented Aug 21, 2015

Fatme Aug 24, 2015

Fatme Aug 25, 2015

teobugslayer Aug 25, 2015

Fatme Aug 25, 2015

teobugslayer commented Aug 25, 2015

Fatme Aug 25, 2015

Use tns_modules from npm #805

Use tns_modules from npm #805

Conversation

e2l3n commented Aug 20, 2015

ns-bot commented Aug 20, 2015

Choose a reason for hiding this comment

teobugslayer commented Aug 20, 2015

Choose a reason for hiding this comment

ligaz commented Aug 20, 2015

Choose a reason for hiding this comment

teobugslayer commented Aug 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e2l3n commented Aug 21, 2015

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

teobugslayer commented Aug 25, 2015

Choose a reason for hiding this comment