Allow local builds when {N} CLI is required as a library #2574

Mitko-Kerezov · 2017-02-24T15:22:27Z

Get rid of dependency in services
Expose build method in platformService which enables building when {N} CLI is required

rosen-vladimirov

I'm unable to prepare a project:

$ tns create app2
$ tns prepare ios
Validating before-prepare arguments.
Hook completed
Preparing project...
TypeError: Cannot read property 'projectName' of undefined
    at IOSProjectService.getXcodeprojPath (/Users/vladimirov/Work/nativescript-cli/lib/services/ios-project-service.ts:696:78)
    at IOSProjectService.getPbxProjPath (/Users/vladimirov/Work/nativescript-cli/lib/services/ios-project-service.ts:725:24)
    at IOSProjectService.createPbxProj (/Users/vladimirov/Work/nativescript-cli/lib/services/ios-project-service.ts:729:39)
    at IOSProjectService.<anonymous> (/Users/vladimirov/Work/nativescript-cli/lib/services/ios-project-service.ts:576:21)
    at Generator.next (<anonymous>)
    at <anonymous> (/Users/vladimirov/Work/nativescript-cli/lib/services/ios-project-service.js:7:65)
    at __awaiter (/Users/vladimirov/Work/nativescript-cli/lib/services/ios-project-service.js:3:12)
    at IOSProjectService.prepareProject (/Users/vladimirov/Work/nativescript-cli/lib/services/ios-project-service.js:528:16)
    at PlatformService.<anonymous> (/Users/vladimirov/Work/nativescript-cli/lib/services/platform-service.ts:255:45)
    at Generator.next (<anonymous>)

I'm unable to build as well:

tns build ios
Executing before-prepare hook from /Users/vladimirov/Work/nativescript-cli/scratch/app2/hooks/before-prepare/nativescript-dev-android-snapshot.js
Preparing project...
Cannot read property 'projectName' of undefined

rosen-vladimirov · 2017-02-28T19:26:09Z

lib/services/platform-service.ts

 		}
 	}
+	@exportedPromise("platformService")
+	public async build(platform: string, platformBuildOptions: IPlatformBuildData, platformTemplate?: string): Promise<void> {


What happened with the idea to place this method in a separate file, for example buildService or localBuildService.
We can $injector resolve it only in the lib bootstrap. I imagine public API like:

require("nativescript").localBuildService.build("android", null); require("nativescript").cloudBuildService.build("android", null);

rosen-vladimirov · 2017-02-28T19:28:16Z

lib/services/platform-service.ts

+	@exportedPromise("platformService")
+	public async build(platform: string, platformBuildOptions: IPlatformBuildData, platformTemplate?: string): Promise<void> {
+		this.$projectData.initializeProjectData(platformBuildOptions.projectDir);
+		await this.preparePlatform(platform, platformBuildOptions, platformTemplate, this.$projectData);


I think we can delete platformTemplate option. It's not used by our customers. The only usage is for our NativeScript developer app. As far as I remember, @TomaNikolov told me that we can safely remove it.
However we can handle this in a separate PR until we are sure we do not need this option.

Given that there are this many changes already I'd personally prefer a separate PR

rosen-vladimirov · 2017-02-28T19:36:06Z

lib/services/platform-service.ts

 	}

-	private getBuildOutputPath(platform: string, platformData: IPlatformData, buildConfig?: IBuildConfig): string {
-		let buildForDevice = buildConfig ? buildConfig.buildForDevice : this.$options.forDevice;
+	private getBuildOutputPath(platform: string, platformData: IPlatformData, buildForDevice: boolean): string {


Maybe you do not want to pass the whole build config, but passing boolean is not my favorite option. I would change the type of buildConfig to object, instead of IBuildConfig:

private getBuildOutputPath(platform: string, platformData: IPlatformData, buildConfig?: { buildForDevice: boolean }): string {

As per the question why passing boolean variable is not a great idea, I would like to remind you for your first PR in our CLIs:
Icenium/icenium-cli#672

rosen-vladimirov · 2017-02-28T19:41:35Z

lib/services/platform-service.ts

 		return !localBuildInfo || !deviceBuildInfo || deviceBuildInfo.buildTime !== localBuildInfo.buildTime;
 	}

-	public async installApplication(device: Mobile.IDevice): Promise<void> {
+	public async installApplication(device: Mobile.IDevice, release: boolean, projectData: IProjectData): Promise<void> {


passing boolean once again... not a merge stopper, but calling the method with:

installApplication(androidDevice, false, projectData);

Makes me curious what does this false do there...

rosen-vladimirov · 2017-02-28T19:42:48Z

lib/services/platform-service.ts

 		} catch (e) {
 			return null;
 		}
 	}

-	private getBuildInfo(platform: string, platformData: IPlatformData, buildConfig: IBuildConfig): IBuildInfo {
-		let buildInfoFilePath = this.getBuildOutputPath(platform, platformData, buildConfig);
+	private getBuildInfo(platform: string, platformData: IPlatformData, buildForDevice: boolean): IBuildInfo {


again

private getBuildInfo(platform: string, platformData: IPlatformData, buildConfig?: { buildForDevice: boolean }): IBuildInfo {

rosen-vladimirov · 2017-02-28T20:32:51Z

lib/commands/emulate.ts

+			clean: this.$options.clean,
+			device: this.$options.device,
+			release: this.$options.release,
+			emulator: this.$options.emulator,


instead of setting the value of this.$options.emulator to true, shouldn't we just set the emulator option here to true?

I decided to reuse the variable 😇

rosen-vladimirov · 2017-02-28T20:38:11Z

lib/commands/plugin/update-plugin.ts

 			pluginNames = installedPlugins.map(p => p.name);
 		}

 		for (let p of pluginNames) {
-			await this.$pluginsService.remove(p);
-			await this.$pluginsService.add(p);
+			await this.$pluginsService.remove(p, this.$projectData);


can you please rename p to plugin or pluginName. p could be anything and it really disturbs me.

rosen-vladimirov · 2017-02-28T20:43:02Z

lib/declarations.ts

+interface INativeScriptDeviceLiveSyncService extends IDeviceLiveSyncServiceBase {
+	/**
+	 * Refreshes the application's content on a device
+	 */


https://i.imgflip.com/1kiquh.jpg

rosen-vladimirov · 2017-02-28T20:43:15Z

lib/declarations.ts

+	/**
+	 * Removes specified files from a connected device
+	 */
+	removeFiles(appIdentifier: string, localToDevicePaths: Mobile.ILocalToDevicePathData[], projectId: string): Promise<void>;


https://i.imgflip.com/1kiquh.jpg

rosen-vladimirov · 2017-02-28T20:46:05Z

lib/options.ts

@@ -41,7 +40,7 @@ export class Options extends commonOptionsLibPath.OptionsBase {
 			chrome: { type: OptionType.Boolean },
 			clean: { type: OptionType.Boolean },
 			watch: { type: OptionType.Boolean, default: true }
-		},
+			},


Includes: * Get rid of $projectData dependency in services * Introduce new service for calling build when required as a library * Emit build output when necessary

Mitko-Kerezov · 2017-03-02T16:00:31Z

Ping @rosen-vladimirov fixed comments and rebased

rosen-vladimirov · 2017-03-03T01:59:23Z

run ci

rosen-vladimirov

After no more than 3 failing QAs tests!
And please monitor the jenkins through the day for possible failures.
Also you should add the localBuildService in the public api tests here: https://github.com/NativeScript/nativescript-cli/blob/master/test/nativescript-cli-lib.ts#L17

Aand information in PublicAPI.md please

Mitko-Kerezov force-pushed the kerezov/local-build-lib branch from 3c1cb0c to d56e0b6 Compare February 27, 2017 09:28

rosen-vladimirov suggested changes Mar 1, 2017

View reviewed changes

Mitko-Kerezov force-pushed the kerezov/local-build-lib branch 2 times, most recently from be043b4 to c9dcae2 Compare March 2, 2017 13:57

Allow local builds when require("")d

a409ffd

Includes: * Get rid of $projectData dependency in services * Introduce new service for calling build when required as a library * Emit build output when necessary

Mitko-Kerezov force-pushed the kerezov/local-build-lib branch from c9dcae2 to f8f29e4 Compare March 2, 2017 15:57

rosen-vladimirov approved these changes Mar 3, 2017

View reviewed changes

Rebase onto master

a5360b1

Mitko-Kerezov force-pushed the kerezov/local-build-lib branch from f8f29e4 to a5360b1 Compare March 3, 2017 10:17

Dimitar Kerezov and others added 3 commits March 3, 2017 12:44

Fix tns init

029bcf9

Fix ios-project-service tests

6c77334

Respect ios-specific options when building

3263762

Mitko-Kerezov merged commit a008db1 into master Mar 3, 2017

Mitko-Kerezov deleted the kerezov/local-build-lib branch March 3, 2017 12:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow local builds when {N} CLI is required as a library #2574

Allow local builds when {N} CLI is required as a library #2574

Mitko-Kerezov commented Feb 24, 2017

rosen-vladimirov left a comment

rosen-vladimirov Feb 28, 2017

rosen-vladimirov Feb 28, 2017

Mitko-Kerezov Mar 1, 2017

rosen-vladimirov Feb 28, 2017

rosen-vladimirov Feb 28, 2017

rosen-vladimirov Feb 28, 2017

rosen-vladimirov Feb 28, 2017

Mitko-Kerezov Mar 2, 2017

rosen-vladimirov Feb 28, 2017

rosen-vladimirov Feb 28, 2017

rosen-vladimirov Feb 28, 2017

rosen-vladimirov Feb 28, 2017

Mitko-Kerezov commented Mar 2, 2017

rosen-vladimirov commented Mar 3, 2017

rosen-vladimirov left a comment

Allow local builds when {N} CLI is required as a library #2574

Allow local builds when {N} CLI is required as a library #2574

Conversation

Mitko-Kerezov commented Feb 24, 2017

rosen-vladimirov left a 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

Mitko-Kerezov commented Mar 2, 2017

rosen-vladimirov commented Mar 3, 2017

rosen-vladimirov left a comment

Choose a reason for hiding this comment