Fix update command and add tests. #3227

KristianDD · 2017-11-22T07:28:33Z

Mitko-Kerezov · 2017-11-24T07:14:40Z

lib/commands/update.ts

+	private backup(tmpDir: string): void {
+		shelljs.rm("-fr", tmpDir);
+		shelljs.mkdir(tmpDir);
+		shelljs.cp(path.join(this.$projectData.projectDir, "package.json"), tmpDir);


you can use PACKAGE_JSON_NAME constant here

Mitko-Kerezov · 2017-11-24T07:21:30Z

lib/commands/update.ts

 		} catch (error) {
 			this.$logger.error("Could not backup project folders!");
+			shelljs.rm("-fr", tmpDir);


It's better to use $fs.deleteDirectory here in order to make use of the dependency injection and it'll be easier to mock

Mitko-Kerezov · 2017-11-24T07:27:41Z

lib/commands/update.ts

+	}
+
+	private restoreBackup(tmpDir: string): void {
+		shelljs.cp("-f", path.join(tmpDir, "package.json"), this.$projectData.projectDir);


you can use $fs.copyFile here

Mitko-Kerezov · 2017-11-24T07:29:25Z

lib/commands/update.ts

+	private restoreBackup(tmpDir: string): void {
+		shelljs.cp("-f", path.join(tmpDir, "package.json"), this.$projectData.projectDir);
+		for (const folder of this.folders) {
+			shelljs.rm("-rf", path.join(this.$projectData.projectDir, folder));


Instead of using shelljs directly it'd be better to introduce a $fs.deleteDirectory method and use it everywhere instead of shelljs.rm

Mitko-Kerezov · 2017-11-24T07:32:40Z

lib/commands/update.ts

+			const folderToCopy = path.join(tmpDir, folder);
+
+			if (this.$fs.exists(folderToCopy)) {
+				shelljs.cp("-fr", folderToCopy, this.$projectData.projectDir);


Same as all other shelljs.cp occurrences - use $fs.copyFile instead

Mitko-Kerezov · 2017-11-24T07:33:49Z

lib/commands/update.ts

+	private backup(tmpDir: string): void {
+		shelljs.rm("-fr", tmpDir);
+		shelljs.mkdir(tmpDir);
+		shelljs.cp(path.join(this.$projectData.projectDir, "package.json"), tmpDir);


Also you can use $fs.copyFile

Mitko-Kerezov · 2017-11-24T07:33:56Z

lib/commands/update.ts

+		for (const folder of this.folders) {
+			const folderToCopy = path.join(this.$projectData.projectDir, folder);
+			if (this.$fs.exists(folderToCopy)) {
+				shelljs.cp("-rf", folderToCopy, tmpDir);


$fs.copyFile

Mitko-Kerezov · 2017-11-24T07:35:12Z

lib/commands/update.ts

-		for (const folder of folders) {
-			shelljs.rm("-fr", folder);
+		for (const folder of this.folders) {
+			shelljs.rm("-rf", path.join(this.$projectData.projectDir, folder));


Same as the other shelljs.rm occurrences

Mitko-Kerezov · 2017-11-24T07:41:03Z

lib/commands/update.ts

-				}
-			}
-
+			this.restoreBackup(tmpDir);
 			this.$logger.error("Could not update the project!");
 		} finally {
 			shelljs.rm("-fr", tmpDir);


As a side note you can replace this shelljs.rm occurrence too

Mitko-Kerezov · 2017-11-24T08:16:11Z

lib/commands/update.ts


-		for (const platform of availablePlatforms) {
+		for (const platform of _.xor(platforms.installed, platforms.packagePlatforms)) {


The logic has changed a bit here - instead of iterating over all availablePlatforms we are basically iterating over packagePlatforms because platforms.packagePlatforms is platforms.installed concatenated with packagePlatforms and the xor will basically only return packagePlatforms.
Is this expected

Yes this is intentional, because we are missing the following part:

if (platformVersion) { packagePlatforms.push(platform); this.$projectDataService.removeNSProperty(this.$projectData.projectDir, platformData.frameworkPackageName); }

The if statement is moved to getPlatforms and we want to call removeNSProperty only for the ones that passed the condition.

The behavior should be the same.

Mitko-Kerezov

Splendid work

KristianDD · 2017-11-29T12:54:17Z

run ci

Plamen5kov

Please add test for invalid version passed to tns update

Plamen5kov

Look at comments

Plamen5kov · 2017-11-29T14:29:56Z

lib/commands/update.ts

@@ -15,49 +15,34 @@ export class UpdateCommand implements ICommand {
 		this.$projectData.initializeProjectData();
 	}

+	static readonly folders: string[] = ["lib", "hooks", "platforms", "node_modules"];


you have constants required anyway, please add lib and hooks there and replace all strings with constants

Plamen5kov · 2017-11-29T14:32:54Z

lib/commands/update.ts

-					shelljs.cp("-rf", folderToCopy, tmpDir);
-				}
-			}
+			this.backup(tmpDir);


better API to understand is this.backup(source, destination), or if you use only source, why do you need a temp folder?

I send the parameter just to avoid repetition of:
const tmpDir = path.join(this.$projectData.projectDir, UpdateCommand.tempFolder);

Plamen5kov · 2017-11-29T14:36:09Z

test/update.ts

+			});
+		});
+
+		it("calls remove for all falders", async () => {


typo: falders

KristianDD added the bug label Nov 22, 2017

KristianDD self-assigned this Nov 22, 2017

Fix update command and add tests.

82641ad

KristianDD force-pushed the kddimitrov/fix-update-command branch from 5107f84 to 82641ad Compare November 22, 2017 17:12

Mitko-Kerezov suggested changes Nov 24, 2017

View reviewed changes

KristianDD added 2 commits November 27, 2017 18:35

Fix comments.

ce93e02

Rewrite tests.

adca13b

KristianDD force-pushed the kddimitrov/fix-update-command branch from f8aa52e to adca13b Compare November 28, 2017 14:36

Mitko-Kerezov approved these changes Nov 28, 2017

View reviewed changes

Plamen5kov self-requested a review November 29, 2017 14:24

Plamen5kov suggested changes Nov 29, 2017

View reviewed changes

Fix comments.

6bf0f6d

KristianDD force-pushed the kddimitrov/fix-update-command branch from aee6ef7 to 6bf0f6d Compare November 30, 2017 10:57

Plamen5kov approved these changes Nov 30, 2017

View reviewed changes

KristianDD merged commit b343e26 into master Nov 30, 2017

KristianDD deleted the kddimitrov/fix-update-command branch November 30, 2017 14:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix update command and add tests. #3227

Fix update command and add tests. #3227

KristianDD commented Nov 22, 2017

Mitko-Kerezov Nov 24, 2017

Mitko-Kerezov Nov 24, 2017

Mitko-Kerezov Nov 24, 2017

Mitko-Kerezov Nov 24, 2017

Mitko-Kerezov Nov 24, 2017

Mitko-Kerezov Nov 24, 2017

Mitko-Kerezov Nov 24, 2017

Mitko-Kerezov Nov 24, 2017

Mitko-Kerezov Nov 24, 2017

Mitko-Kerezov Nov 24, 2017

KristianDD Nov 27, 2017

Mitko-Kerezov left a comment

KristianDD commented Nov 29, 2017

Plamen5kov left a comment

Plamen5kov left a comment

Plamen5kov Nov 29, 2017

Plamen5kov Nov 29, 2017

KristianDD Nov 29, 2017

Plamen5kov Nov 29, 2017


		for (const platform of availablePlatforms) {
		for (const platform of _.xor(platforms.installed, platforms.packagePlatforms)) {

Fix update command and add tests. #3227

Fix update command and add tests. #3227

Conversation

KristianDD commented Nov 22, 2017

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 left a comment

Choose a reason for hiding this comment

KristianDD commented Nov 29, 2017

Plamen5kov left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment