tns-android/ios are installed as dev dependencies #2532

Plamen5kov · 2017-02-13T15:39:59Z

Previously tns-android and tns-ios have been installed in nativescript key in the root package.json, which is illogical since these platforms are just regular npm packages.
With this PR, both platforms are installed as devDependencies and the package.json looks like:

{
  "nativescript": {
    "id": "org.nativescript.<app_name>"
  },
  "dependencies": {
    ....
  },
  "devDependencies": {
    "tns-android": "^2.5.0"
  }
}

This change does not introduce any breaking changes, but it's a first step towards future improvements.

previously tns-android and tns-ios have been installed in nativescript key in the root package.json now both platforms are installed as a dev-dependencies cli will generate a productionDependencies.json that will be respected by gradle, so only the production dependencies are traversed, platform package is left installed as a dev dependency

rosen-vladimirov

I have some concerns regarding these changes.

As tns-android is written in package.json, we can remove magic dependencies installed by CLI when adding android platform: https://github.com/NativeScript/nativescript-cli/blob/master/lib/services/android-project-service.ts#L14-L19 - they can be added as dependencies of tns-android package. This way CLI will not take care of installing them.
What happens when the user modifies the version of tns-android/tns-ios in devDependencies and executes any CLI command after that? Should we execute clean prepare?
Related to the point above - how to determine the version of Android/iOS that I have in platforms dir? As now I have tns-android in my package.json, but I've modified it's version, how can I determine in the package installed in node_modules is the same as the one I have in my platforms dir.
I've tried to test the changes, but it looks like the value in package.json is not respected. I've executed:

$ tns create app1
$ cd app1
$ tns platform add android

This added the following line to my devDependencies section in package.json:

 "tns-android": "^2.5.0"

After that I've removed node_modules and platforms directories and modified the line in package.json to be:

 "tns-android": "~2.0.0"

After that I've tried tns prepare android, but CLI didn't respect my version and installed tns-android 2.5.0. It also modified my package.json back to:

 "tns-android": "^2.5.0"

Is this expected?

rosen-vladimirov · 2017-02-26T19:19:06Z

lib/services/platform-service.ts

@@ -82,7 +82,7 @@ export class PlatformService implements IPlatformService {
 		let packageToInstall = "";
 		let npmOptions: IStringDictionary = {


can we add --save-exact to npmOptions, as currently we are adding the framework package with ^, for example:

"tns-android": "^2.5.0"

This means that once we have tns-android 2.6.0, and I execute rm -rf node_modules && npm i I'll get the new framework.

rosen-vladimirov · 2017-02-26T19:24:23Z

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

@@ -126,6 +126,14 @@ export class NodeModulesBuilder implements INodeModulesBuilder {
 		let dependenciesBuilder = this.$injector.resolve(NodeModulesDependenciesBuilder, {});
 		let productionDependencies = dependenciesBuilder.getProductionDependencies(this.$projectData.projectDir);

+		let prodDependenciesArr = [];


The for below can be replaced with map:

const prodDependenciesArr = _.map(productionDependencies, prodDep => prodDep.name);

rosen-vladimirov · 2017-02-26T19:25:18Z

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

+		}
+
+		let prodDependenciesFilePath = path.join(this.$projectData.projectDir, "platforms", "android", "productionDependencies.json");
+		this.$fs.writeJson(prodDependenciesFilePath, prodDependenciesArr);


what's the purpose of this file? Why should we write it to platforms dir? Can you add comment in the code, please.

I'll add comments to the code, but the basic idea is, we need to tell gradle what are the production dependencies, that CLI found, so we search for dependencies only in production modules, it saves time, and it's less error prone.

Why gradle needs to know which are the production dependencies? As far as I remember, CLI copies only production dependencies to platforms/android dir.
Also... this code is executed for iOS builds as well.

Because gradle needs to go through the node_modules folder and get all .aar so it can include them into the build. Currently we traverse all of the node_modules which is error prone and it's a waste of time, when we know which dependencies we should use.

rosen-vladimirov · 2017-02-26T21:41:24Z

Another thing came to my mind, that could be a showstopper at the moment - tns-ios package can be installed only on macOS. So in case I work with my project on macOS and test the iOS version, I'll end up with tns-ios in the devDependencies. After that, anyone else will be unable to use the same project on other OS, as the npm install command will fail.
Another problem that I see (but maybe it's not such an issue), is that if you have both runtimes in devDependencies and you would like to build your project for Android for example, you'll have both runtimes in node_modules (as npm install will be called at the root fo the project). This will slow down the process a little bit, but I think we can live with it.

Plamen5kov · 2017-02-27T07:27:13Z

@rosen-vladimirov

After that, anyone else will be unable to use the same project on other OS, as the npm install command will fail.

Haven't thought about that. Maybe we can use --force as a workaround in this particular case, or add additional logic to be able to build for android at least if possible.

After that I've tried tns prepare android, but CLI didn't respect my version and installed tns-android 2.5.0. It also modified my package.json back to: 2.5.0. Is this expected?

It's not expected, I'll look into it.

As tns-android is written in package.json, we can remove magic dependencies installed by CLI when adding android platform: https://github.com/NativeScript/nativescript-cli/blob/master/lib/services/android-project-service.ts#L14-L19 - they can be added as dependencies of tns-android package. This way CLI will not take care of installing them.

I agree, it would be a great thing to add template dependencies to the actual template. Will do it, for sure.

Related to the point above - how to determine the version of Android/iOS that I have in platforms dir? As now I have tns-android in my package.json, but I've modified it's version, how can I determine in the package installed in node_modules is the same as the one I have in my platforms dir.

I believe we should always check the version of the platform from the package.json in node_modules/tns-android(ios) so on that point, on a change in the tns-android(ios) packages we can do a platform remove -> add so we update/downgrade to what the user has specified in the package.json. This way the project will stay consistent: package.json version == node_modules/tns-android(ios) version == platforms/android(ios) version.

rosen-vladimirov · 2017-02-27T07:54:15Z

@Plamen5kov

After that, anyone else will be unable to use the same project on other OS, as the npm install command will fail.
Haven't thought about that. Maybe we can use --force as a workaround in this particular case, or add additional logic to be able to build for android at least if possible.

The problem is that npm install command will fail in this case. So l suggest enabling installation of tns-ios on Windows (modification of the tns-ios package).

dtopuzov · 2017-03-07T13:25:42Z

run ci

rosen-vladimirov · 2017-03-20T21:32:08Z

@Plamen5kov if this PR won't make it for 3.0.0, maybe we should close it, what do you think?

Plamen5kov requested a review from rosen-vladimirov February 13, 2017 15:40

Plamen5kov self-assigned this Feb 13, 2017

Plamen5kov removed the request for review from rosen-vladimirov February 14, 2017 11:23

Plamen5kov force-pushed the plamen5kov/install_platform_as_devdependency branch from 7576191 to 5a3266a Compare February 14, 2017 12:44

Plamen5kov mentioned this pull request Feb 14, 2017

gradle will respect productionDependencies.json file NativeScript/android#708

Closed

rosen-vladimirov suggested changes Feb 26, 2017

View reviewed changes

pkoleva mentioned this pull request Feb 27, 2017

tns run android fails (2.5.0) #2467

Closed

petekanev mentioned this pull request Mar 13, 2017

Error: spawn E2BIG #2561

Closed

Plamen5kov closed this Mar 21, 2017

Plamen5kov deleted the plamen5kov/install_platform_as_devdependency branch March 28, 2017 16:22

Plamen5kov restored the plamen5kov/install_platform_as_devdependency branch March 28, 2017 16:22

Plamen5kov deleted the plamen5kov/install_platform_as_devdependency branch March 28, 2017 16:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tns-android/ios are installed as dev dependencies #2532

tns-android/ios are installed as dev dependencies #2532

Plamen5kov commented Feb 13, 2017

rosen-vladimirov left a comment

rosen-vladimirov Feb 26, 2017

rosen-vladimirov Feb 26, 2017

rosen-vladimirov Feb 26, 2017 •

edited

Loading

Plamen5kov Feb 27, 2017

rosen-vladimirov Feb 27, 2017

Plamen5kov Feb 27, 2017

rosen-vladimirov commented Feb 26, 2017

Plamen5kov commented Feb 27, 2017

rosen-vladimirov commented Feb 27, 2017

dtopuzov commented Mar 7, 2017

rosen-vladimirov commented Mar 20, 2017

		@@ -82,7 +82,7 @@ export class PlatformService implements IPlatformService {
		let packageToInstall = "";
		let npmOptions: IStringDictionary = {

tns-android/ios are installed as dev dependencies #2532

tns-android/ios are installed as dev dependencies #2532

Conversation

Plamen5kov commented Feb 13, 2017

rosen-vladimirov left a comment

Choose a reason for hiding this comment

rosen-vladimirov Feb 26, 2017

Choose a reason for hiding this comment

rosen-vladimirov Feb 26, 2017

Choose a reason for hiding this comment

rosen-vladimirov Feb 26, 2017 • edited Loading

Choose a reason for hiding this comment

Plamen5kov Feb 27, 2017

Choose a reason for hiding this comment

rosen-vladimirov Feb 27, 2017

Choose a reason for hiding this comment

Plamen5kov Feb 27, 2017

Choose a reason for hiding this comment

rosen-vladimirov commented Feb 26, 2017

Plamen5kov commented Feb 27, 2017

rosen-vladimirov commented Feb 27, 2017

dtopuzov commented Mar 7, 2017

rosen-vladimirov commented Mar 20, 2017

rosen-vladimirov Feb 26, 2017 •

edited

Loading