Skip to content

Fix update command and add tests. #3227

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

Merged
merged 4 commits into from
Nov 30, 2017
Merged

Fix update command and add tests. #3227

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
82641ad
Fix update command and add tests.
KristianDD Nov 20, 2017
ce93e02
Fix comments.
KristianDD Nov 27, 2017
adca13b
Rewrite tests.
KristianDD Nov 28, 2017
6bf0f6d
Fix comments.
KristianDD Nov 30, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 64 additions & 42 deletions lib/commands/update.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,49 +15,34 @@ export class UpdateCommand implements ICommand {
this.$projectData.initializeProjectData();
}

private folders: string[] = ["lib", "hooks", "platforms", "node_modules"];
private tempFolder: string = ".tmp_backup";

public async execute(args: string[]): Promise<void> {
const folders = ["lib", "hooks", "platforms", "node_modules"];
const tmpDir = path.join(this.$projectData.projectDir, ".tmp_backup");
const tmpDir = path.join(this.$projectData.projectDir, this.tempFolder);

try {
shelljs.rm("-fr", tmpDir);
shelljs.mkdir(tmpDir);
shelljs.cp(path.join(this.$projectData.projectDir, "package.json"), tmpDir);
for (const folder of folders) {
const folderToCopy = path.join(this.$projectData.projectDir, folder);
if (this.$fs.exists(folderToCopy)) {
shelljs.cp("-rf", folderToCopy, tmpDir);
}
}
this.backup(tmpDir);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return;
}

try {
await this.executeCore(args, folders);
await this.executeCore(args);
} catch (error) {
shelljs.cp("-f", path.join(tmpDir, "package.json"), this.$projectData.projectDir);
for (const folder of folders) {
shelljs.rm("-rf", path.join(this.$projectData.projectDir, folder));

const folderToCopy = path.join(tmpDir, folder);

if (this.$fs.exists(folderToCopy)) {
shelljs.cp("-fr", folderToCopy, this.$projectData.projectDir);
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

public async canExecute(args: string[]): Promise<boolean> {
for (const arg of args) {
const platform = arg.split("@")[0];
this.$platformService.validatePlatformInstalled(platform, this.$projectData);
const platforms = this.getPlatforms();

for (const platform of platforms.packagePlatforms) {
const platformData = this.$platformsData.getPlatformData(platform, this.$projectData);
const platformProjectService = platformData.platformProjectService;
await platformProjectService.validate(this.$projectData);
Expand All @@ -66,42 +51,79 @@ export class UpdateCommand implements ICommand {
return args.length < 2 && this.$projectData.projectDir !== "";
}

private async executeCore(args: string[], folders: string[]): Promise<void> {
let platforms = this.$platformService.getInstalledPlatforms(this.$projectData);
const availablePlatforms = this.$platformService.getAvailablePlatforms(this.$projectData);
const packagePlatforms: string[] = [];
private async executeCore(args: string[]): Promise<void> {
const platforms = this.getPlatforms();

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

await this.$platformService.removePlatforms(platforms, this.$projectData);
await this.$platformService.removePlatforms(platforms.installed, this.$projectData);
await this.$pluginsService.remove("tns-core-modules", this.$projectData);
await this.$pluginsService.remove("tns-core-modules-widgets", this.$projectData);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the other shelljs.rm occurrences

}

platforms = platforms.concat(packagePlatforms);
if (args.length === 1) {
for (const platform of platforms) {
for (const platform of platforms.packagePlatforms) {
await this.$platformService.addPlatforms([platform + "@" + args[0]], this.$options.platformTemplate, this.$projectData, this.$options, this.$options.frameworkPath);
}

await this.$pluginsService.add("tns-core-modules@" + args[0], this.$projectData);
} else {
await this.$platformService.addPlatforms(platforms, this.$options.platformTemplate, this.$projectData, this.$options, this.$options.frameworkPath);
await this.$platformService.addPlatforms(platforms.packagePlatforms, this.$options.platformTemplate, this.$projectData, this.$options, this.$options.frameworkPath);
await this.$pluginsService.add("tns-core-modules", this.$projectData);
}

await this.$pluginsService.ensureAllDependenciesAreInstalled(this.$projectData);
}

private getPlatforms(): {installed: string[], packagePlatforms: string[]} {
const installedPlatforms = this.$platformService.getInstalledPlatforms(this.$projectData);
const availablePlatforms = this.$platformService.getAvailablePlatforms(this.$projectData);
const packagePlatforms: string[] = [];

for (const platform of availablePlatforms) {
const platformData = this.$platformsData.getPlatformData(platform, this.$projectData);
const platformVersion = this.$projectDataService.getNSValue(this.$projectData.projectDir, platformData.frameworkPackageName);
if (platformVersion) {
packagePlatforms.push(platform);
}
}

return {
installed: installedPlatforms,
packagePlatforms: installedPlatforms.concat(packagePlatforms)
};
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can use $fs.copyFile here

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


const folderToCopy = path.join(tmpDir, folder);

if (this.$fs.exists(folderToCopy)) {
shelljs.cp("-fr", folderToCopy, this.$projectData.projectDir);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
}

private backup(tmpDir: string): void {
shelljs.rm("-fr", tmpDir);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$fs.removeDirectory

shelljs.mkdir(tmpDir);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$fs.createDirectory

shelljs.cp(path.join(this.$projectData.projectDir, "package.json"), tmpDir);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can use PACKAGE_JSON_NAME constant here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also you can use $fs.copyFile

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$fs.copyFile

}
}
}
}

$injector.registerCommand("update", UpdateCommand);
2 changes: 2 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@
"analyze": true,
"devDependencies": {
"@types/chai": "4.0.1",
"@types/sinon": "4.0.0",
"@types/chai-as-promised": "0.0.31",
"@types/chokidar": "1.6.0",
"@types/lockfile": "1.0.0",
Expand All @@ -104,6 +105,7 @@
"grunt-ts": "6.0.0-beta.16",
"istanbul": "0.4.5",
"mocha": "3.1.2",
"sinon": "4.1.2",
"should": "7.0.2",
"source-map-support": "^0.4.14",
"tslint": "5.4.3",
Expand Down
Loading