feat: make app_resources's path configurable #3329

petekanev · 2018-01-22T12:08:51Z

Refactor the project-data service to read a config.json for the 'appResources' property entry, which points relatively to the App_Resources directory.

All references to app/App_Resources now retrieve the path from the said service.
To keep the service backward compatible, if no config.json is present, or the appResources entry isn't available, location defaults to projectDir/app/App_Resources.
Change the app-files-updater class to not collect the App_Resources directory when enumerating through the user's source files. Instead a separate method will enumerate the App_Resources directory and its files, and will copy it in platforms/**/app. I find that behavior to be more predictable, otherwise enumerating just the myProject/app directory, could skip enumerating the App_Resources if they are located someplace else, as declared in the nsconfig.json file.

KristianDD · 2018-02-15T12:02:11Z

run ci

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

run ci

rosen-vladimirov · 2018-02-20T20:14:38Z

lib/services/app-files-updater.ts

 	}

 	protected resolveAppSourceFiles(): string[] {
-		// Copy all files from app dir, but make sure to exclude tns_modules
+		// Copy all files from app dir, but make sure to exclude tns_modules and App_Resources


why is this change applied? Can you describe it in the commit message

rosen-vladimirov · 2018-02-20T21:42:40Z

test/stubs.ts

 		this.projectDir = this.projectDir || projectDir;
 	}
+	public getAppResourcesDirectoryPath(projectDir?: string): string {


this stub uses the same implementation from the real class, by duplicating the code. Is there a particular reason for this?

I found it easier to test this way, since I would always get the default path to the App_Resources. Ideally I could always resolve the path to the default app/App_Resources for the purposes of testing.

rosen-vladimirov · 2018-02-20T21:48:21Z

test/ios-project-service.ts

@@ -65,12 +65,13 @@ function createTestInjector(projectPath: string, projectName: string): IInjector
 	testInjector.register("iOSEntitlementsService", IOSEntitlementsService);
 	testInjector.register("logger", LoggerLib.Logger);
 	testInjector.register("options", OptionsLib.Options);
-	testInjector.register("projectData", {
+	const projectData = Object.assign({}, ProjectDataStub, {


maybe you can just mock the required methods, but this is not a merge stopper

rosen-vladimirov · 2018-02-20T21:49:17Z

test/ios-entitlements-service.ts

@@ -46,7 +47,7 @@ describe("IOSEntitlements Service Tests", () => {
 		injector = createTestInjector();

 		platformsData = injector.resolve("platformsData");
-		projectData = <IProjectData>platformsData.getPlatformData();
+		projectData = injector.resolve("projectData");


you can use:

projectData = $injector.resolve<IProjectData>("projectData");

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

lib/project-data.ts

+			if (configNS && configNS[constants.CONFIG_NS_APP_RESOURCES_ENTRY]) {
+				const appResourcesDirPath = configNS[constants.CONFIG_NS_APP_RESOURCES_ENTRY];
+
+				absoluteAppResourcesDirPath = path.resolve(projectDir, appResourcesDirPath);


is it worth checking if this path exists?

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

lib/project-data.ts

@@ -87,6 +88,27 @@ 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);
 	}

+	public getAppResourcesDirectoryPath(projectDir?: string): string {
+		if (!projectDir) {
+			projectDir = this.projectDir;


if initializeProjectData has not been called, the this.projectDir will also be undefined and the path.join below will fail.

Correct, I am unsure how to tackle this. The need for the additional projectDir parameter arose when using the getter during project creation. What would you recommend I check here?

I would just return null here. The caller should decide what to do.

rosen-vladimirov · 2018-02-20T22:12:11Z

lib/constants.ts

@@ -28,6 +28,8 @@ 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";


I prefer naming json properties in camelCase (appResources in this case), but I do not have a strong opinion here. @KristianDD , @Mitko-Kerezov what do you think?

I'll follow the convention seemingly established in package.json and tsconfig and go with camelCase.

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

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

@@ -36,6 +36,7 @@ export class PreparePlatformJSService extends PreparePlatformService implements
 	public async preparePlatform(config: IPreparePlatformJSInfo): Promise<void> {
 		if (!config.changesInfo || config.changesInfo.appFilesChanged || config.changesInfo.changesRequirePrepare) {
 			await this.copyAppFiles(config);
+			this.copyAppResourcesFiles(config);


I assume this is related to https://github.com/NativeScript/nativescript-cli/pull/3329/files#diff-d7315453ac31262b45d0c2530d69cd94R47 change. Can you describe a little bit more what is the idea

rosen-vladimirov · 2018-02-20T23:15:03Z

@Pip3r4o I've tried the following commands with the current branch and some warnings are shown from gradle:

$ tns create myApp
$ cd myApp
$ tns build android
$ mv app/App_Resources my_app_resources
$ tns build android -
Warning is printed: 
No manifest found in `<path to project>\app\App_Resources\Android\AndroidManifest.xml`, but build continues.
$ vi nsconfig.json // set the following content { "app_resources": "my_app_resources" }
$ tns build android
Preparing project...
Project successfully prepared (android)
Building project...
Gradle build...
         + couldn't load user-defined configuration from D:\Work\nativescript-cli\scratch\app74\app\App_Resources\Android\app.gradle. File doesn't exist.
         + adding nativescript runtime package dependency: nativescript-optimized
         + adding aar plugin dependency: D:\Work\nativescript-cli\scratch\app74\node_modules\tns-core-modules-widgets\platforms\android\widgets-release.aar
false
Running full build
Project successfully built.

petekanev · 2018-02-21T08:03:53Z

@rosen-vladimirov the gradle build script from the android-runtime release and master branches currently reads the app.gradle from a hardcoded path to the app_resources. There's an additional PR that uses a method similar to getAppResourcesDirectoryPath in order to locate the App_Resources dir.

KristianDD

If I remember correctly after testing this PR the provided path inside nsconfig.json should end with "App_Resources" otherwise the CLI will fail to build the project.

We should either
1 Fix it - find where APP_RESOURCES_FOLDER_NAME is used and make sure it is used only for the destination of the resources inside platforms folder.

2 Validate the path provided, but this will make the user experience worse.

petekanev · 2018-02-22T11:48:20Z

@rosen-vladimirov @KristianDD I made some changes after taking your comments into consideration. Please review.

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

lib/project-data.ts

@@ -103,10 +107,17 @@ export class ProjectData implements IProjectData {
 				const appResourcesDirPath = configNS[constants.CONFIG_NS_APP_RESOURCES_ENTRY];

 				absoluteAppResourcesDirPath = path.resolve(projectDir, appResourcesDirPath);
+
+				if (this.$fs.exists(absoluteAppResourcesDirPath)) {


Check what happens if we go through here

nativescript-cli/lib/services/project-service.ts

Line 113 in 27e937b

private async ensureAppResourcesExist(projectDir: string): Promise<void> {

@KristianDD
Creating an ng/ts project, which would suggest that 2 templates will be downloaded - one before ensureAppResourcesExist is called, the other one - afterwards, doesn't make the process crash. I am not sure if the case would be the same if the template is shipped with nsconfig.json which points to App_Resources.

What do you think? Should I perhaps return the path, even if the directory doesn't exist, as that's where the App_Resources from the template should be extracted?

KristianDD · 2018-02-22T14:51:13Z

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;
+		const appResourcesDirInSources = path.join(this.appSourceDirectoryPath, path.basename(appResourcesPath));


Check if it works with appResourcesPath directly.

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

petekanev · 2018-02-26T08:12:00Z

Closing this PR in favor of #3356 which includes the changes presented here.

petekanev requested review from KristianDD, Mitko-Kerezov and rosen-vladimirov January 22, 2018 12:08

petekanev force-pushed the pete/nsconfig-json branch 3 times, most recently from 7dcfa3a to 64a973d Compare January 25, 2018 09:41

petekanev mentioned this pull request Feb 1, 2018

feat: introduce resources update command #3347

Merged

KristianDD mentioned this pull request Feb 8, 2018

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

Merged

petekanev force-pushed the pete/nsconfig-json branch from d0b0311 to 510d564 Compare February 20, 2018 08:04

rosen-vladimirov suggested changes Feb 20, 2018

View reviewed changes

KristianDD suggested changes Feb 21, 2018

View reviewed changes

petekanev mentioned this pull request Feb 22, 2018

[Proposal] List of version 4.0 (breaking?) changes NativeScript/android#890

Closed

KristianDD reviewed Feb 22, 2018

View reviewed changes

petekanev and others added 5 commits February 22, 2018 17:59

refactor(project-data): make appResourcesDirectoryPath getter

ce3c173

chore(appresorces): fix tests

7b93bb7

chore(tests) fix xcconfig test

9888581

fix ts compile error

b557596

petekanev force-pushed the pete/nsconfig-json branch 2 times, most recently from dc0cdaf to 990ec08 Compare February 22, 2018 16:18

refactor: address pr comments, replace app_resources usage for whatever

b9890bf

the name of the app_resources directory basename is

petekanev force-pushed the pete/nsconfig-json branch from 990ec08 to b9890bf Compare February 22, 2018 16:24

petekanev closed this Feb 26, 2018

petekanev deleted the pete/nsconfig-json branch March 8, 2018 09:48

Uh oh!

feat: make app_resources's path configurable #3329

feat: make app_resources's path configurable #3329

Uh oh!

Conversation

petekanev commented Jan 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KristianDD commented Feb 15, 2018

Uh oh!

dtopuzov commented Feb 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rosen-vladimirov commented Feb 20, 2018

Uh oh!

petekanev commented Feb 21, 2018

Uh oh!

KristianDD left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petekanev commented Feb 22, 2018

Uh oh!

KristianDD Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petekanev commented Feb 26, 2018

Uh oh!

Uh oh!

petekanev commented Jan 22, 2018 •

edited

Loading

KristianDD left a comment •

edited

Loading

KristianDD Feb 22, 2018 •

edited

Loading