Kddimitrov/nsconfig app folder #3356

KristianDD · 2018-02-08T11:10:29Z

Part of implementation for #3257

KristianDD · 2018-02-12T15:36:44Z

run ci

sis0k0 · 2018-02-14T08:21:46Z

lib/project-data.ts

+		return absoluteAppResourcesDirPath || path.join(this.getAppDirectoryPath(projectDir), constants.APP_RESOURCES_FOLDER_NAME);
+	}
+
+	public getAppDirectoryPath(projectDir?: string): string {


This method seems to be very similar to the one above (getAppResourcesDirectoryPath). Do you think it would be a good idea to make a common method that accepts the property name as an argument? Or maybe have a method that parses the config file and validates it?

dtopuzov · 2018-02-15T12:26:11Z

run ci

rosen-vladimirov · 2018-02-20T22:35:14Z

lib/project-data.ts

+		this.errorInvalidProject(projectDir);
+	}
+
+	private errorInvalidProject(projectDir: string) {


this method needs return type

rosen-vladimirov · 2018-02-20T22:36:37Z

lib/project-data.ts

+			return this.nsConfig[constants.CONFIG_NS_APP_RESOURCES_ENTRY];
+		}
+
+		return path.join(this.getAppDirectoryRelativePath(), constants.APP_RESOURCES_FOLDER_NAME);


you can shorten this:

return this.nsConfig && this.nsConfig[constants.CONFIG_NS_APP_RESOURCES_ENTRY] || path.join(this.getAppDirectoryRelativePath(), constants.APP_RESOURCES_FOLDER_NAME);

I consider mine more readable and structured. Will change it if you insist.

no, not a problem ;)

rosen-vladimirov · 2018-02-20T22:37:37Z

lib/project-data.ts

+			return this.nsConfig[constants.CONFIG_NS_APP_ENTRY];
+		}
+
+		return constants.APP_FOLDER_NAME;


same as above - you can shorten the code:

return this.nsConfig && this.nsConfig[constants.CONFIG_NS_APP_ENTRY] || constants.APP_FOLDER_NAME;

rosen-vladimirov · 2018-02-20T22:38:09Z

lib/project-data.ts

+
+	public getAppDirectoryRelativePath(): string {
+		if (this.nsConfig && this.nsConfig[constants.CONFIG_NS_APP_ENTRY]) {
+			return this.nsConfig[constants.CONFIG_NS_APP_ENTRY];


what will happen in case the path in the nsconfig.json is not relative?

I think we should add validation for this as supporting paths not relative to the project will be very hard in cloud builds. Maybe we should also change the key inside nsconfig.json to appRelativePath or something in this lines.

rosen-vladimirov · 2018-02-20T22:43:14Z

lib/project-data.ts

+		this.errorInvalidProject(projectDir);
+	}
+
+	public initializeProjectDataFromContent(packageJsonContent: string, nsconfigContent: string, projectDir?: string): void {


@sis0k0 is it ok to pass the content as string or you'll need a method that accepts the parsed json object (i.e. the parsed nsconfig.json) itself?

I think it's better to pass the content as string and let the project data service do the parsing/validation.

KristianDD · 2018-02-22T14:04:39Z

lib/constants.ts

@@ -28,6 +28,9 @@ export const BUILD_DIR = "build";
 export const OUTPUTS_DIR = "outputs";
 export const APK_DIR = "apk";
 export const RESOURCES_DIR = "res";
+export const CONFIG_NS_FILE_NAME = "nsconfig.json";
+export const CONFIG_NS_APP_RESOURCES_ENTRY = "app_resources";
+export const CONFIG_NS_APP_ENTRY = "app_folder";


Change to camelCase and add Path suffix at the end

KristianDD · 2018-02-22T14:06:48Z

lib/project-data.ts

-					return;
-				}
+				let packageJsonContent: any = null;
+				packageJsonContent = this.$fs.readText(projectFilePath);


Check with invalid location (no package.json file)

KristianDD · 2018-02-22T14:18:52Z

lib/services/livesync/livesync-service.ts

@@ -527,6 +527,8 @@ export class LiveSyncService extends EventEmitter implements IDebugLiveSyncServi
 			}
 		}

+		patterns.push(projectData.appResourcesDirectoryPath);


Check what happens if app resources are still inside app folder.

KristianDD · 2018-02-22T14:22:44Z

lib/services/project-service.ts

@@ -101,7 +101,7 @@ export class ProjectService implements IProjectService {
 	private async extractTemplate(projectDir: string, realTemplatePath: string): Promise<void> {
 		this.$fs.ensureDirectoryExists(projectDir);

-		const appDestinationPath = path.join(projectDir, constants.APP_FOLDER_NAME);
+		const appDestinationPath = this.$projectData.getAppDirectoryPath(projectDir);


Check usages here. What happens if template has existing nsconfig.json inside.

Templates currently contain only the contents of the app folder. No option to add nsconfig in template for the moment.

KristianDD · 2018-02-22T14:36:36Z

lib/project-data.ts

+	}
+
+	public getAppDirectoryPath(projectDir?: string): string {
+		if (!projectDir) {


return null if missing projectDir

Fatme · 2018-02-23T21:46:58Z

lib/definitions/project.d.ts

@@ -63,12 +63,18 @@ interface IProjectData extends IProjectDir {
 	appDirectoryPath: string;
 	appResourcesDirectoryPath: string;
 	projectType: string;
+	nsConfig: any;


Can we use some interface here? - for example INsConfig

Fatme · 2018-02-23T21:49:24Z

lib/project-data.ts

-					return;
-				}
+				let packageJsonContent: any = null;
+				packageJsonContent = this.$fs.readText(projectFilePath);


Just let packageJsonContent = this.$fs.readText(projectFilePath). There is no need to declare the variable on a saparate line.

Fatme · 2018-02-23T21:52:11Z

lib/project-data.ts

-				}
+				let packageJsonContent: any = null;
+				packageJsonContent = this.$fs.readText(projectFilePath);
+				const nsConfigContent: any = this.getNsConfigContent(projectDir);


No need to specify the type. It will be automatically inferred by typescript compiler.

Fatme · 2018-02-23T21:56:23Z

lib/project-data.ts

+		return path.join(this.getAppDirectoryRelativePath(), constants.APP_RESOURCES_FOLDER_NAME);
+	}
+
+	public getAppDirectoryPath(projectDir?: string): string {


The logic in this method is very similiar to getAppResourcesDirectoryPath(). Can we try to extract the common part?

Fatme · 2018-02-23T22:01:43Z

lib/services/prepare-platform-js-service.ts

@@ -101,6 +102,13 @@ export class PreparePlatformJSService extends PreparePlatformService implements
 			this.$errors.failWithoutHelp(`Processing node_modules failed. ${error}`);
 		}
 	}
+
+	private copyAppResourcesFiles(config: IPreparePlatformJSInfo) {


Missing return type

rosen-vladimirov · 2018-02-25T22:01:05Z

lib/project-data.ts

+		const projectFilePath = this.getProjectFilePath(projectDir);
+		// If no project found, projectDir should be null
+		let nsData: any = null;
+		let nsConfig: any = null;


we definitely need an interface for nsConfig and the data that can be included in it. Can you please add a new one and use it as a type here instead of any.

rosen-vladimirov · 2018-02-25T22:08:30Z

lib/services/app-files-updater.ts

+		// exclude the app_resources directory from being enumerated
+		// for copying if it is present in the application sources dir
+		const appResourcesPath = projectData.appResourcesDirectoryPath;
+		sourceFiles = sourceFiles.filter(dirName => !path.normalize(dirName).startsWith(path.normalize(appResourcesPath)));


you can extract the path.normalize(appResourcesPath) in a separate variable before applying the filtering. This way you'll not calculate it on each iteration.

rosen-vladimirov · 2018-02-25T22:15:29Z

lib/bootstrap.ts

@@ -6,7 +6,7 @@ $injector.require("options", "./options");
 $injector.require("nativescript-cli", "./nativescript-cli");

 $injector.require("projectData", "./project-data");
-$injector.require("projectDataService", "./services/project-data-service");
+$injector.requirePublic("projectDataService", "./services/project-data-service");


When you add a new method to PublicApi, you have to:

Update the documentation: https://github.com/NativeScript/nativescript-cli/blob/master/PublicAPI.md

Add the service and the exposed methods in the nativescript-cli-lib tests

NOTE: More information is available here

rosen-vladimirov · 2018-02-25T22:20:55Z

lib/services/project-data-service.ts

@@ -41,6 +42,13 @@ export class ProjectDataService implements IProjectDataService {
 		return projectDataInstance;
 	}

+	@exported("projectDataService")


can you export getProjectData as well - it will be used from Sidekick to get paths to app and App_Resources directories

KristianDD · 2018-02-26T13:50:31Z

lib/services/prepare-platform-js-service.ts

+		const appDestinationDirectoryPath = path.join(config.platformData.appDestinationDirectoryPath, constants.APP_FOLDER_NAME);
+		const appResourcesSourcePath = config.projectData.appResourcesDirectoryPath;
+
+		shell.cp("-Rf", appResourcesSourcePath, appDestinationDirectoryPath);


Shouldn't we use fileSystem module?

KristianDD · 2018-02-26T16:56:51Z

run ci

Refactor the project-data service to read a config.json for the 'app_resources' property entry, which points relatively to the App_Resources directory. All references to app/App_Resources now retrieve the path from said service. To keep the service backwards compatible, if no config.json is present, or the app_resources entry isn't available, location defaults to projectDir/app/App_Resources.

the name of the app_resources directory basename is

… and tests

sis0k0 · 2018-02-28T13:38:33Z

lib/project-data.ts

+		this.errorInvalidProject(projectDir);
+	}
+
+	public initializeProjectDataFromContent(packageJsonContent: string, nsconfigContent: string, projectDir?: string): void {


Isn't nsconfigContent optional?

Yes. I have missed to make it optional as we don't use it in the codebase.

KristianDD · 2018-02-28T13:58:47Z

run ci

KristianDD mentioned this pull request Feb 8, 2018

feat(nsconfig): read app resources path from nsconfig or default NativeScript/android#944

Merged

Mitko-Kerezov mentioned this pull request Feb 9, 2018

Get app directory location from CLI NativeScript/nativescript-dev-webpack#436

Closed

KristianDD force-pushed the kddimitrov/nsconfig-app-folder branch from 0dea0cd to 461f597 Compare February 12, 2018 15:20

sis0k0 reviewed Feb 14, 2018

View reviewed changes

KristianDD force-pushed the kddimitrov/nsconfig-app-folder branch from 8901a1b to ef0f8c2 Compare February 20, 2018 18:58

rosen-vladimirov suggested changes Feb 20, 2018

View reviewed changes

KristianDD force-pushed the kddimitrov/nsconfig-app-folder branch 2 times, most recently from aff7576 to 09973b6 Compare February 22, 2018 12:43

KristianDD commented Feb 22, 2018

View reviewed changes

KristianDD self-assigned this Feb 22, 2018

KristianDD force-pushed the kddimitrov/nsconfig-app-folder branch 2 times, most recently from a3a2814 to 8c302a3 Compare February 23, 2018 13:34

Fatme reviewed Feb 25, 2018

View reviewed changes

rosen-vladimirov suggested changes Feb 25, 2018

View reviewed changes

petekanev mentioned this pull request Feb 26, 2018

feat: make app_resources's path configurable #3329

Closed

petekanev self-assigned this Feb 26, 2018

KristianDD commented Feb 26, 2018

View reviewed changes

petekanev force-pushed the kddimitrov/nsconfig-app-folder branch from cd5267e to 79d744b Compare February 26, 2018 15:43

KristianDD force-pushed the kddimitrov/nsconfig-app-folder branch from ae4f213 to b96a8cc Compare February 27, 2018 13:15

This was referenced Feb 27, 2018

Android build fails when App_Resources folder is renamed #3400

Closed

Renaming App_Resources folder leads to failure in loading the app resources #3401

Closed

rosen-vladimirov approved these changes Feb 27, 2018

View reviewed changes

petekanev mentioned this pull request Feb 28, 2018

fix(prepare): disregard app_resources's dir name when preparing in platforms/ #3403

Merged

petekanev and others added 9 commits February 28, 2018 13:44

refactor(project-data): make appResourcesDirectoryPath getter

21abc36

chore(appresorces): fix tests

3eebb1e

chore(tests) fix xcconfig test

28f2f95

fix ts compile error

1aba3c1

refactor: address pr comments, replace app_resources usage for whatever

1122be6

the name of the app_resources directory basename is

feat(nsconfig): read app folder path from nsconfig

487c80c

chore(nsconfig): fix comments

9c44985

chore(public api): add projectDataService to public api documentation…

2b5ef94

… and tests

KristianDD force-pushed the kddimitrov/nsconfig-app-folder branch from b96a8cc to 2b5ef94 Compare February 28, 2018 12:01

sis0k0 reviewed Feb 28, 2018

View reviewed changes

KristianDD merged commit 00e7a30 into master Feb 28, 2018

KristianDD deleted the kddimitrov/nsconfig-app-folder branch June 18, 2018 11:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kddimitrov/nsconfig app folder #3356

Kddimitrov/nsconfig app folder #3356

KristianDD commented Feb 8, 2018

KristianDD commented Feb 12, 2018

sis0k0 Feb 14, 2018

dtopuzov commented Feb 15, 2018

rosen-vladimirov Feb 20, 2018

rosen-vladimirov Feb 20, 2018

KristianDD Feb 21, 2018

rosen-vladimirov Feb 25, 2018

rosen-vladimirov Feb 20, 2018

rosen-vladimirov Feb 20, 2018

KristianDD Feb 21, 2018 •

edited

Loading

rosen-vladimirov Feb 20, 2018

sis0k0 Feb 21, 2018

KristianDD Feb 22, 2018 •

edited

Loading

KristianDD Feb 22, 2018

KristianDD Feb 22, 2018

KristianDD Feb 22, 2018

KristianDD Feb 26, 2018

KristianDD Feb 22, 2018

Fatme Feb 23, 2018

Fatme Feb 23, 2018

Fatme Feb 23, 2018

Fatme Feb 23, 2018

KristianDD Feb 27, 2018

Fatme Feb 23, 2018

rosen-vladimirov Feb 25, 2018

rosen-vladimirov Feb 25, 2018

rosen-vladimirov Feb 25, 2018

KristianDD Feb 27, 2018

rosen-vladimirov Feb 25, 2018

KristianDD Feb 26, 2018

KristianDD commented Feb 26, 2018

sis0k0 Feb 28, 2018

KristianDD Feb 28, 2018

KristianDD commented Feb 28, 2018

Kddimitrov/nsconfig app folder #3356

Kddimitrov/nsconfig app folder #3356

Conversation

KristianDD commented Feb 8, 2018

KristianDD commented Feb 12, 2018

Choose a reason for hiding this comment

dtopuzov commented Feb 15, 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

Choose a reason for hiding this comment

KristianDD Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KristianDD Feb 22, 2018 • 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

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 Feb 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KristianDD commented Feb 28, 2018

KristianDD Feb 21, 2018 •

edited

Loading

KristianDD Feb 22, 2018 •

edited

Loading