Kddimitrov/add platform application identifier setting #3120

KristianDD · 2017-09-13T12:06:21Z

DO NOT MERGE

Allow adding separate application identifiers for each platform inside package.json

Description

While still overwrite of applicationId from app.gradle is possible, user is able to specify a platform specific identifier inside package.json. This addresses problems related to livesync not working when applicationId is changed inside app.gradle.

Does your commit message include the wording below to reference a specific issue in this repo?

Fixes #3040.

Related Pull Requests

NativeScript/android#848

Does your pull request have [unit tests]

KristianDD · 2017-09-18T15:03:18Z

lib/services/project-service.ts

@@ -80,7 +80,7 @@ export class ProjectService implements IProjectService {
 	public isValidNativeScriptProject(pathToProject?: string): boolean {
 		try {
 			this.$projectData.initializeProjectData(pathToProject);
-			return !!this.$projectData.projectDir && !!this.$projectData.projectId;
+			return !!this.$projectData.projectDir && !!this.$projectData.projectIdentifiers;


Should check this. Not sure if we should verify for both iOS and android identifiers here or only for at least one.

Plamen5kov · 2017-10-02T11:36:51Z

lib/project-data.ts

+	public projectIdentifiers: IProjectIdentifier;
+	get projectId(): string {
+		this.warnProjectId();
+		return this.projectIdentifiers.ios;


why does a common module project-data.ts return only an iOS project identifier?

This is intended to let hooks and extension that use the old property to behave as before.
Before if the user changed the application identifier for Android he must use the gradle file and the rest of the code will still use the one inside the package.json (the one for iOS).

maybe put a comment in the code, because it looks a bit strange returning only ios identifier in a common file

Will do that.

rosen-vladimirov · 2017-09-26T13:11:57Z

lib/definitions/debug.d.ts

@@ -5,7 +5,7 @@ interface IDebugData extends Mobile.IDeviceIdentifier {
 	/**
 	 * Application identifier of the app that it will be debugged.
 	 */
-	applicationIdentifier: string;
+	applicationIdentifier: IProjectIdentifier;


I think this is a breaking change for the PublicAPI

Will check what this affects and whether we will need to add new property and change this to setter and getter.

rosen-vladimirov · 2017-09-26T13:15:52Z

lib/project-data.ts

@@ -87,6 +97,25 @@ export class ProjectData implements IProjectData {
 		this.$errors.fail("No project found at or above '%s' and neither was a --path specified.", projectDir || this.$options.path || currentDir);
 	}

+	private getProjectIdentifiers(data :any): IProjectIdentifier {


maybe you can use data: string | IProjectIdentifier

Plamen5kov · 2017-10-02T11:42:05Z

lib/project-data.ts

+	}
+	set projectId(identifier: string) {
+		this.warnProjectId();
+		this.projectIdentifiers.ios = identifier;


is there a validation for the identifier somewhere before this line? (check if valid ios identifier)

No the validation happens at a later stage.

I think validation should happen as early as possible, but it's up to you.

Plamen5kov · 2017-10-02T11:42:25Z

lib/project-data.ts

+	set projectId(identifier: string) {
+		this.warnProjectId();
+		this.projectIdentifiers.ios = identifier;
+		this.projectIdentifiers.android = identifier;


is this a valid android identifier?

No the validation happens at a later stage.

it's up to you, but as I mentioned earlier validation should happen as early as possible so other modules can be sure validation is already done

Plamen5kov · 2017-10-02T11:47:33Z

lib/project-data.ts

+		this.warnProjectId();
+		return this.projectIdentifiers.ios;
+	}
+	set projectId(identifier: string) {


is setting project id used only in initializeProjectData?

Its not used but its intended for hooks and extensions that might try to modify it.

please add a comment as this is not easily deducted

Plamen5kov · 2017-10-02T11:49:09Z

lib/project-data.ts

+				android: data,
+				ios: data
+			};
+		} else {


What's the case when the android and ios package identifiers should be set to an empty string? Shouldn't we throw an error/warning here? (nvm, I got this is a default setting, but rename method)

Plamen5kov · 2017-10-02T11:51:53Z

lib/project-data.ts

@@ -87,6 +97,25 @@ export class ProjectData implements IProjectData {
 		this.$errors.fail("No project found at or above '%s' and neither was a --path specified.", projectDir || this.$options.path || currentDir);
 	}

+	private getProjectIdentifiers(data :any): IProjectIdentifier {


since this is a private function, used only once, why don't we change the signature to:
private getProjectIdentifiers(projectIdentifier :string): IProjectIdentifier

Rosen is right that the argument might be a string and an object. This depends on the use case of the user - if he needs different identifiers for Android and iOS the argument will be object.

In this case please rename data: any to projectIdentifierInfo: any or something better, but let it not be just data

Plamen5kov · 2017-10-02T11:54:21Z

lib/services/android-project-service.ts

 		} catch (e) {
-			this.$logger.warn(`\n${e}.\nCheck if you're using an outdated template and update it.`);
+			// Ignore


I think we should at least trace here, so we don't forget to remove this code later on

Well I left a comment to remind us to remove this peace of code. If you consider tracing is better I can remove the comment. Do you prefer both the comment and trace?

Personally, I prefer trace, because it see it anytime I'm debugging as opposed to looking at the code (not so often)

Plamen5kov · 2017-10-02T12:00:26Z

lib/project-data.ts

@@ -70,7 +80,7 @@ export class ProjectData implements IProjectData {
 					this.projectFilePath = projectFilePath;
 					this.appDirectoryPath = path.join(projectDir, constants.APP_FOLDER_NAME);
 					this.appResourcesDirectoryPath = path.join(projectDir, constants.APP_FOLDER_NAME, constants.APP_RESOURCES_FOLDER_NAME);
-					this.projectId = data.id;
+					this.projectIdentifiers = this.getProjectIdentifiers(data.id);


please rename getProjectIdentifiers to something that better describes what the method does

Not sure I understand what the naming of the method should be. I am open to suggestions.

my suggestion is initializeProjectIdentifiers

Plamen5kov · 2017-10-02T12:03:16Z

lib/services/android-project-service.ts

@@ -375,7 +376,7 @@ export class AndroidProjectService extends projectServiceBaseLib.PlatformProject
 			const filesForInterpolation = this.$fs.enumerateFilesInDirectorySync(resourcesDestinationDirectoryPath, file => this.$fs.getFsStats(file).isDirectory() || path.extname(file) === constants.XML_FILE_EXTENSION) || [];
 			for (const file of filesForInterpolation) {
 				this.$logger.trace(`Interpolate data for plugin file: ${file}`);
-				await this.$pluginVariablesService.interpolate(pluginData, file, projectData);
+				await this.$pluginVariablesService.interpolate(pluginData, file, projectData, projectData.projectIdentifiers.android);


why do you pass projectData.projectIdentifiers.android as an additional parameter when you already pass projectData?

Because pluginVariablesService doesn't have the platform context to get the proper identifier for the case.

[Optional] I agree that it is really strange to pass both projectData and property from projectData. In this case I would suggest to pass projectData.projectDir and projectData.projectIdentifiers.android as two args and remove passing of projectData itself.

Plamen5kov · 2017-10-02T12:16:58Z

lib/services/debug-service.ts

-			this.$errors.failWithoutHelp(`The application ${debugData.applicationIdentifier} is not installed on device with identifier ${debugData.deviceIdentifier}.`);
+		const devicePlatform = device.deviceInfo.platform;
+
+		if (!(await device.applicationManager.isApplicationInstalled(debugData.applicationIdentifier[devicePlatform.toLowerCase()]))) {


please change all the toLowerCase() all over the PR to a commonplace, wherever it makes sence

Well I discussed the options with @PanayotCankov and @rosen-vladimirov and this was the least bad way to handle it.

The other was to make a getter and setter for the lowercase properties inside an implementation of IProjectIdentifier.

Given the platforms and other constants are living only in our CLI code, I don't understand the issue of not constantly using toLowerCase?

Plamen5kov · 2017-10-02T12:19:18Z

lib/services/ios-debug-service.ts

@@ -118,7 +118,7 @@ export class IOSDebugService extends DebugServiceBase implements IPlatformDebugS
 			waitForDebugger: true,
 			captureStdin: true,
 			args: args,
-			appId: debugData.applicationIdentifier,
+			appId: debugData.applicationIdentifier.ios,


ideally this should be named applicationIdentifiers plural

Plamen5kov · 2017-10-02T12:38:12Z

lib/services/livesync/livesync-service.ts

@@ -671,6 +672,10 @@ export class LiveSyncService extends EventEmitter implements IDebugLiveSyncServi
 		}
 	}

+	public emitLivesyncEvent (event: string, livesyncData: ILivesyncEventData): boolean {


is this only a rename of the function?

Not exactly. There is still a calls to this.emit that made the introduction of ILivesyncEventData very ugly(all of the properties had to be optional).

and why was the introduction of ILivesyncEventData necessary?

So that we have types of the properties of the data object. I was going to miss it and break Sidekick.

Plamen5kov · 2017-10-02T12:45:48Z

lib/services/platform-service.ts

@@ -236,13 +236,21 @@ export class PlatformService extends EventEmitter implements IPlatformService {
 			platform = this.$mobileHelper.normalizePlatformName(platform);
 			this.$logger.trace("Validate options for platform: " + platform);
 			const platformData = this.$platformsData.getPlatformData(platform, projectData);
-			return await platformData.platformProjectService.validateOptions(projectData.projectId, provision, teamId);
+			return await platformData.platformProjectService.validateOptions(
+				projectData.projectIdentifiers[platform.toLowerCase()],


why don't we just validate both project identifiers?

Because its not relevant to validate android identifier when trying to run the application on iOS device,

Codewise it kind of makes sense, but I get what you mean.

Plamen5kov · 2017-10-02T12:46:35Z

lib/services/platform-service.ts

-			return await platformData.platformProjectService.validateOptions(projectData.projectId, provision, teamId);
+			return await platformData.platformProjectService.validateOptions(
+				projectData.projectIdentifiers[platform.toLowerCase()],
+				provision,


a bit strange to pass provision in a common file like platform-service.ts when it's iOS specific. Is there a better way?

Will check it, but will probably fix in separate PR.

Plamen5kov · 2017-10-02T12:47:23Z

lib/services/platform-service.ts

 		} else {
 			let valid = true;
 			for (const availablePlatform in this.$platformsData.availablePlatforms) {
 				this.$logger.trace("Validate options for platform: " + availablePlatform);
 				const platformData = this.$platformsData.getPlatformData(availablePlatform, projectData);
-				valid = valid && await platformData.platformProjectService.validateOptions(projectData.projectId, provision, teamId);
+				valid = valid && await platformData.platformProjectService.validateOptions(
+					projectData.projectIdentifiers[availablePlatform.toLowerCase()],


same comments as above

Plamen5kov

Please take a look at the comments.
Because

platfrom = ...
 ... = projectIdenfiers[platform]

is a repeating pattern, is there a way to extract to common place?

KristianDD · 2017-10-10T11:15:39Z

lib/services/android-project-service.ts

-		if (projectData && projectData.projectId) {
-			const idParts = projectData.projectId.split(".");
+		if (projectData && projectData.projectIdentifiers && projectData.projectIdentifiers.android) {
+			const idParts = projectData.projectIdentifiers.android.split(".");


@Plamen5kov(accidently deleted the comment) Yes there is validation for the identifier.

miroslavaivanova · 2017-11-13T09:08:01Z

run ci

mpochron · 2018-08-25T12:03:43Z

@KristianDD what status now?

rosen-vladimirov · 2018-09-17T13:31:31Z

lib/services/livesync/livesync-service.ts

@@ -792,6 +795,10 @@ export class LiveSyncService extends EventEmitter implements IDebugLiveSyncServi
 		}
 	}

+	public emitLivesyncEvent (event: string, livesyncData: ILiveSyncEventData): boolean {
+		return this.emit(event, livesyncData);


Maybe we can add some trace message before emitting the event.

Will tracing the event name be enough or should we JSON.stringify the livesyncData?

You can use:

this.$logger.trace(`Will emit event ${event} with data`, livesyncData);

rosen-vladimirov · 2018-09-17T13:32:48Z

lib/definitions/livesync.d.ts

+	deviceIdentifier: string,
+	applicationIdentifier?: string,
+	projectDir: string,
+	syncedFiles?: Object


is this really an object?

My bad. It is a string[].

KristianDD · 2018-09-19T08:08:50Z

run ci

KristianDD · 2018-09-19T11:51:59Z

run ci

Rename projectId to projectIdentifiers. projectId is left as getter and setter to avoid breaking change in hooks and CLI extensions Fix ui tests for platform service and safeguard missing id. Remove breaking change from debug data. Use IProjectDefinition from mobile-cli-lib Fix comments. Fix comments chore: send projectDir instead of projectData to PluginVariableService. chore: fix comments chore: fix tests

kspearrin · 2018-09-20T16:56:02Z

Any idea what version this will land in?

rosen-vladimirov · 2018-09-20T16:59:00Z

Hey @kspearrin ,
We'll release this in NativeScript 5.0.0, but you can try it even now by installing CLI's next version:

npm i -g nativescript@next

KristianDD force-pushed the kddimitrov/add-platform-application-identifier-setting branch 2 times, most recently from 63a0357 to bca04be Compare September 13, 2017 15:01

KristianDD mentioned this pull request Sep 13, 2017

Take applicationId from package.json NativeScript/android#848

Merged

KristianDD force-pushed the kddimitrov/add-platform-application-identifier-setting branch from bca04be to 6f2df11 Compare September 15, 2017 11:29

KristianDD commented Sep 18, 2017

View reviewed changes

KristianDD force-pushed the kddimitrov/add-platform-application-identifier-setting branch 3 times, most recently from 2d62804 to ad87f08 Compare September 25, 2017 14:41

Plamen5kov reviewed Oct 2, 2017

View reviewed changes

rosen-vladimirov reviewed Oct 2, 2017

View reviewed changes

Plamen5kov reviewed Oct 2, 2017

View reviewed changes

Plamen5kov suggested changes Oct 2, 2017

View reviewed changes

NativeScript deleted a comment from Plamen5kov Oct 10, 2017

KristianDD commented Oct 10, 2017

View reviewed changes

KristianDD force-pushed the kddimitrov/add-platform-application-identifier-setting branch from 6c00599 to b62a907 Compare October 23, 2017 14:00

KristianDD force-pushed the kddimitrov/add-platform-application-identifier-setting branch from 81e987f to 1814c4b Compare November 8, 2017 15:33

Natalia-Hristova modified the milestone: 4.2.0 Jul 6, 2018

rosen-vladimirov modified the milestones: 4.2.0, 4.3.0 Jul 17, 2018

KristianDD force-pushed the kddimitrov/add-platform-application-identifier-setting branch from 002f8a0 to 228893d Compare September 4, 2018 13:45

NativeScript deleted a comment from miroslavaivanova Sep 10, 2018

KristianDD force-pushed the kddimitrov/add-platform-application-identifier-setting branch from 228893d to 663736e Compare September 10, 2018 15:16

NativeScript deleted a comment from miroslavaivanova Sep 11, 2018

KristianDD force-pushed the kddimitrov/add-platform-application-identifier-setting branch from ea73872 to 098d545 Compare September 17, 2018 09:44

rosen-vladimirov approved these changes Sep 17, 2018

View reviewed changes

KristianDD force-pushed the kddimitrov/add-platform-application-identifier-setting branch from 098d545 to 64085d8 Compare September 18, 2018 15:00

KristianDD added 3 commits September 20, 2018 16:13

tests: fix tests

7813560

chore: fix comments

0576a20

KristianDD force-pushed the kddimitrov/add-platform-application-identifier-setting branch from 64085d8 to 0576a20 Compare September 20, 2018 13:14

KristianDD changed the title ~~DO NOT MERGE Kddimitrov/add platform application identifier setting~~ Kddimitrov/add platform application identifier setting Sep 20, 2018

KristianDD merged commit 2be786c into master Sep 20, 2018

KristianDD deleted the kddimitrov/add-platform-application-identifier-setting branch September 20, 2018 14:31

madmas mentioned this pull request Oct 4, 2018

add switch to set projectName from package.json #3863

Closed

5 tasks

Kddimitrov/add platform application identifier setting #3120

Kddimitrov/add platform application identifier setting #3120

Conversation

KristianDD commented Sep 13, 2017 • edited Loading

Allow adding separate application identifiers for each platform inside package.json

Description

Does your commit message include the wording below to reference a specific issue in this repo?

Related Pull Requests

Does your pull request have [unit tests]

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 Oct 2, 2017 • 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

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 Oct 2, 2017 • 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

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 Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Plamen5kov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miroslavaivanova commented Nov 13, 2017

mpochron commented Aug 25, 2018

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

KristianDD commented Sep 19, 2018

KristianDD commented Sep 19, 2018

kspearrin commented Sep 20, 2018

rosen-vladimirov commented Sep 20, 2018

KristianDD commented Sep 13, 2017 •

edited

Loading

Plamen5kov Oct 2, 2017 •

edited

Loading

Plamen5kov Oct 2, 2017 •

edited

Loading

Plamen5kov Oct 2, 2017 •

edited

Loading