feat: introduce resources update command #3347

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merged

petekanev merged 2 commits into master from pete/introduce-resources-update-command

Mar 12, 2018

Contributor

petekanev commented Feb 1, 2018 •

edited

Loading

Introduce the 'tns resources update android' command. By design, upon execution it should migrate the directory structure of App_Resources/Android to the new v4 structure - the one that supports the inclusion of java source files, arbitrary assets, and any resource files in the App_Resources/Android/src/main directory structure. Additional, user-defined flavors can also be created taking advantage of the new dir structure.

petekanev force-pushed the pete/introduce-resources-update-command branch 2 times, most recently from e7a77fd to 057ce07 Compare

February 7, 2018 14:35

petekanev force-pushed the pete/nsconfig-json branch from d0b0311 to 510d564 Compare

February 20, 2018 08:04

petekanev mentioned this pull request

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

Closed

petekanev force-pushed the pete/nsconfig-json branch 3 times, most recently from 990ec08 to b9890bf Compare

February 22, 2018 16:24

petekanev force-pushed the pete/introduce-resources-update-command branch from 057ce07 to 2eae80d Compare

March 1, 2018 14:01

petekanev changed the base branch from pete/nsconfig-json to master

March 1, 2018 14:02

petekanev force-pushed the pete/introduce-resources-update-command branch 4 times, most recently from 9dd55f8 to b1aa8dd Compare

March 1, 2018 14:45

petekanev requested review from rosen-vladimirov, KristianDD and Mitko-Kerezov

March 1, 2018 14:46

rosen-vladimirov suggested changes

View reviewed changes

docs/man_pages/project/configuration/resources-update.md Outdated

		@@ -0,0 +1,27 @@
		resources-update

Contributor

rosen-vladimirov Mar 1, 2018

I think you have to add some jekyll specific logic, ping @etabakov for further info

Contributor

etabakov Mar 2, 2018

That's right. You need to put this header, in order the article to be part of docs.nativescript.org (otherwise, it will be just skipped):

<% if (isJekyll) { %>---
title: <some title>
position: <order number>
---<% } %>

You can take a look at any of the other articles for an example.

As a side note, put a # sign before the title to make it look ok in html.

lib/commands/resources-update.ts Outdated

+              		for (const platform of args) {
+              			if (!this.$androidResourcesV4MigrationService.canMigrate(platform)) {
+              				this.$errors.failWithoutHelp("The iOS platform does not need to have its resources updated.");

Contributor

rosen-vladimirov Mar 2, 2018

You cannot be sure that user had stated "iOS" as a platform. I would use

`The ${platform} does not need...

lib/declarations.d.ts Outdated

@@ @@ -497,6 +497,12 @@ interface IInfoService { @@
               	printComponentsInfo(): Promise<void>;
               }
+              interface IAndroidResourcesV4MigrationService {

Contributor

rosen-vladimirov Mar 2, 2018

the name is somehow misleading. In case we need additional resources update in the future we will have to create new service. Can we use IAndroidResourcesMigrationService, so all future migrations will be included in this service?

lib/services/android-project-service.ts Outdated

+              		}
+              	}
+              	private getAppResourcesDestinationDirectoryPathOldAppResourcesDirStructure(projectData: IProjectData): string {

Contributor

rosen-vladimirov Mar 2, 2018

private methods should be last - after all public and protected. Also this name is a little bit confusing. I think you should remove the Structure from its name - it returns path, not a structure.
Maybe getOldAppResourcesDestinationDirPath
And the next method can be called getNewAppResourcesDestinationDirPath

However, old and new will not mean a lot after six months, maybe we can name the methods:

getLegacyAppResourcesDestinationDirPath
getUpdatedAppResourcesDestinationDirPath

lib/services/android-project-service.ts Outdated


		let stringsFilePath: string;

		if (!this.$androidResourcesV4MigrationService.hasMigrated(appResourcesDirectoryPath)) {

Contributor

rosen-vladimirov Mar 2, 2018

can we switch the if - it is easier for reading in case there's not ! in the if:

if (this.$androidResourcesV4MigrationService.hasMigrated(appResourcesDirectoryPath)) {
    stringsFilePath = path.join(this.getAppResourcesDestinationDirectoryPath(projectData), "main", "res", 'values', 'strings.xml');
} else {
    stringsFilePath = path.join(this.getAppResourcesDestinationDirectoryPath(projectData), 'values', 'strings.xml');
}

lib/services/project-v4-migration-service.ts Outdated

+              	async migrate(appResourcesDir: string): Promise<void> {
+              		const originalAppResources = path.join(appResourcesDir, AndroidResourcesV4MigrationService.ANDROID_DIR);
+              		const appResourcesDestination = path.join(appResourcesDir, AndroidResourcesV4MigrationService.ANDROID_DIR_TEMP);
+              		const appMainSourceSet = path.join(appResourcesDestination, "src", "main");

Contributor

rosen-vladimirov Mar 2, 2018

src and main from constants maybe

lib/services/project-v4-migration-service.ts Outdated

+              		const originalAppResources = path.join(appResourcesDir, AndroidResourcesV4MigrationService.ANDROID_DIR);
+              		const appResourcesDestination = path.join(appResourcesDir, AndroidResourcesV4MigrationService.ANDROID_DIR_TEMP);
+              		const appMainSourceSet = path.join(appResourcesDestination, "src", "main");
+              		const appResourcesMainSourceSetResourcesDestination = path.join(appMainSourceSet, "res");

Contributor

rosen-vladimirov Mar 2, 2018

res -> RESOURCES_DIR from constants

lib/services/project-v4-migration-service.ts Outdated

+              			this.$fs.readDirectory(source).map(name => path.join(source, name)).filter(isDirectory);
+              		shell.cp(path.join(originalAppResources, "app.gradle"), path.join(appResourcesDestination, "app.gradle"));
+              		shell.cp(path.join(originalAppResources, "AndroidManifest.xml"), path.join(appMainSourceSet, "AndroidManifest.xml"));

Contributor

rosen-vladimirov Mar 2, 2018

AndroidManifest.xml -> MANIFEST_FILE_NAME from constants

lib/services/project-v4-migration-service.ts Outdated

+              		const resourceDirectories = getDirectories(originalAppResources);
+              		resourceDirectories.forEach(dir => {
+              			shell.cp("-Rf", dir, appResourcesMainSourceSetResourcesDestination);

Contributor

rosen-vladimirov Mar 2, 2018

fs.copyFile

lib/services/project-v4-migration-service.ts Outdated

+              		// move the new, updated app_resources to App_Resources/Android, as  the de facto resources
+              		shell.mv(appResourcesDestination, originalAppResources);
+              		this.$logger.out(`Successfully updated your project's App_Resources/Android directory structure.\nThe previous version of App_Resources/Android has been renamed to App_Resources/${AndroidResourcesV4MigrationService.ANDROID_DIR_OLD}`);

Contributor

rosen-vladimirov Mar 2, 2018

Is it possible for the App_Resources dir to be called something else? For example in case I name it "my_resources" and use nsconfig.json, this message may be misleading.

Contributor Author

petekanev commented Mar 2, 2018

@rosen-vladimirov thanks for the review. I'll address your comments and come back to you when it's ready.

petekanev mentioned this pull request

Adding file to Android build at app/src/main/assets for getAssets() NativeScript/android#700

Closed

petekanev force-pushed the pete/introduce-resources-update-command branch 3 times, most recently from 6b6c5fa to 6cee992 Compare

March 5, 2018 15:32

petekanev added this to the 4.0.0 milestone

petekanev force-pushed the pete/introduce-resources-update-command branch from 6cee992 to 5e6b255 Compare

March 6, 2018 14:09

petekanev self-assigned this

petekanev force-pushed the pete/introduce-resources-update-command branch from 5e6b255 to 7fe0380 Compare

March 7, 2018 13:09

KristianDD reviewed

View reviewed changes

lib/services/android-project-service.ts Outdated

+              		let stringsFilePath: string;
+              		if (this.$androidResourcesMigrationService.hasMigrated(appResourcesDirectoryPath)) {
+              			stringsFilePath = path.join(this.getAppResourcesDestinationDirectoryPath(projectData), constants.MAIN_DIR, constants.RESOURCES_DIR, 'values', 'strings.xml');

Contributor

KristianDD Mar 7, 2018

Can extract this.getAppResourcesDestinationDirectoryPath(projectData) to a variable outside the if statements.

petekanev force-pushed the pete/introduce-resources-update-command branch 3 times, most recently from 4228eeb to 88d3f24 Compare

March 8, 2018 13:47

KristianDD reviewed

View reviewed changes

lib/services/android-project-service.ts Outdated

+              		if (this.$fs.exists(resourcesDirPath)) {
+              			const resourcesDirs = this.$fs.readDirectory(resourcesDirPath).filter(resDir => !resDir.match(valuesDirRegExp));
+              			_.each(resourcesDirs, resourceDir => {
+              				this.$fs.deleteDirectory(path.join(this.getAppResourcesDestinationDirectoryPath(projectData), resourceDir));

Contributor

KristianDD Mar 8, 2018

Can extract getAppResourcesDestinationDirectoryPath outside of the each statement.

petekanev force-pushed the pete/introduce-resources-update-command branch from 88d3f24 to 216a83d Compare

March 8, 2018 14:49

KristianDD suggested changes

View reviewed changes

lib/services/android-resources-migration-service.ts Outdated

+              		const getDirectories = (source: string) =>
+              			this.$fs.readDirectory(source).map(name => path.join(source, name)).filter(isDirectory);
+              		this.$fs.copyFile(path.join(originalAppResources, "app.gradle"), path.join(appResourcesDestination, "app.gradle"));

Contributor

KristianDD Mar 8, 2018

Please take in consideration other files that might be inside the root of the App_Resources directory like "settings.gradle" and "settings.josn".

lib/services/prepare-platform-native-service.ts Outdated

+              				if (appResourcesDirStructureHasMigrated) {
+              					this.$fs.copyFile(path.join(appResourcesAndroid, "src", "*"), appResourcesDestination);
+              					this.$fs.deleteDirectory(path.join(appResourcesDestination, "libs"));

Contributor

KristianDD Mar 8, 2018

This seems unnecessary for migrated projects.

lib/services/android-resources-migration-service.ts Outdated

+              		const resourceDirectories = getDirectories(originalAppResources);
+              		resourceDirectories.forEach(dir => {
+              			this.$fs.copyFile(dir, appResourcesMainSourceSetResourcesDestination);

Contributor

KristianDD Mar 8, 2018

Skip "libs" folder when coping resources.

petekanev force-pushed the pete/introduce-resources-update-command branch from 216a83d to 829b97a Compare

March 8, 2018 16:42

Contributor Author

petekanev commented Mar 8, 2018

@KristianDD I addressed those comments.

petekanev force-pushed the pete/introduce-resources-update-command branch from 829b97a to 9a7cc04 Compare

March 9, 2018 07:52

KristianDD approved these changes

View reviewed changes

Contributor

miroslavaivanova commented Mar 12, 2018

run ci

rosen-vladimirov reviewed

View reviewed changes

lib/commands/resources-update.ts Outdated

+              	}
+              }
+              $injector.registerCommand("resources-update", ResourcesUpdateCommand);

Contributor

rosen-vladimirov Mar 12, 2018

can you rename the command to resources|migrate. This will allow us to have a command hierarchical structure with this PR, where we can name the commands resources|generate.

Contributor Author

petekanev Mar 12, 2018

Sure!

Contributor Author

petekanev Mar 12, 2018

Done.

KristianDD reviewed

View reviewed changes

docs/man_pages/index.md Outdated

@@ @@ -32,6 +32,7 @@ Command | Description @@
               [platform list](project/configuration/platform.html) | Lists all platforms that the project currently targets.
               [platform&nbsp;remove&nbsp;`<Platform>`](project/configuration/platform-remove.html) | Removes the selected platform from the platforms that the project currently targets. This operation deletes all platform-specific files and subdirectories from your project.
               [platform update `<Platform>`](project/configuration/platform-update.html) | Updates the NativeScript runtime for the specified platform.
+              [resources-update](project/configuration/resources-update.html) | Updates the App_Resources/<platform>'s internal folder structure to conform to that of an Android project.

Contributor

KristianDD Mar 12, 2018 •

edited

Loading

Just an idea - " conform to that of an Android project." might sound more concrete if we change "Android project" to "Android Native project" or "Android Studio project".

KristianDD reviewed

View reviewed changes

docs/man_pages/project/configuration/resources-update.md Outdated

+              `$ tns resources-update` | Defaults to executing `$ tns resources-update android`. Updates the App_Resources/Android's folder structure.
+              `$ tns resources-update android` | Updates the App_Resources/Android's folder structure.
+              Updates the App_Resources/<platform>'s internal folder structure to conform to that of an Android project. Android resource files and directories will be located at the following paths:

Contributor

KristianDD Mar 12, 2018

Same as above. If we change it, update here too.

petekanev force-pushed the pete/introduce-resources-update-command branch from 9a7cc04 to a5d89f4 Compare

March 12, 2018 12:13

petekanev added 2 commits

March 12, 2018 14:16


          feat(resources-update-command): introduce resources-update command

87c4541

Introduce the 'tns resources-update android' command. By design, upon execution it should migrate the directory structure of App_Resources/Android to the new v4 structure - the one that supports inclusion of java source files, arbitrary assets, and any resource files in the App_Resources/Android/src/main directory structure. Additional, user-defined flavors can also be created taking advantage of the new dir structure.

docs(resources-update-command): add documentation for the new resources-update command

fix(resources-update-command): make prepare and run backward-compatible

fix(resource-update-command-tests): inject the new service in tests

chore: address PR comments

chore: fix git rebase error


          docs(resourdces|update): update resources update command doc

2031a8c

petekanev force-pushed the pete/introduce-resources-update-command branch from a5d89f4 to 2031a8c Compare

March 12, 2018 12:18

rosen-vladimirov approved these changes

View reviewed changes

Contributor

rosen-vladimirov left a comment

Great Work @Pip3r4o !

petekanev merged commit 5f2d77b into master

petekanev deleted the pete/introduce-resources-update-command branch

March 12, 2018 13:29

lambourn mentioned this pull request

nativescript-dev-assets conflicts with NativeScript 4.0 project layout nativescript-community/ui-lottie#21

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet