Skip to content

Refactor project and platform services #7

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 12 commits into from
Jul 24, 2014
Merged

Refactor project and platform services #7

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
bef4e47
Extract projectData as separate class
Jul 18, 2014
0284cf8
Refactor project and platform services
Fatme Jul 19, 2014
6828871
Remove unneeded code
Fatme Jul 19, 2014
0f36cf2
Refactor buildProject
Fatme Jul 19, 2014
5f12de3
Refactor prepareProject
Fatme Jul 20, 2014
3f42363
add normalizedPlatformName in platformData
Fatme Jul 20, 2014
0bc9d78
Code review changes
Fatme Jul 21, 2014
c0b11cf
More code review changes
Fatme Jul 22, 2014
b962f30
Remove platform-project-service
Fatme Jul 23, 2014
ebf4c97
Add missing dependencies in package.json
Fatme Jul 23, 2014
29a5190
More code review changes
Fatme Jul 23, 2014
598d079
Code review improvements
Fatme Jul 24, 2014
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
5 changes: 5 additions & 0 deletions lib/bootstrap.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,16 @@ require("./common/bootstrap");

$injector.require("nativescript-cli", "./nativescript-cli");

$injector.require("projectData", "./services/project-service");
$injector.require("projectService", "./services/project-service");
$injector.require("androidProjectService", "./services/project-service");
$injector.require("iOSProjectService", "./services/project-service");

$injector.require("projectTemplatesService", "./services/project-templates-service");

$injector.require("platformsData", "./services/platform-service");
$injector.require("platformService", "./services/platform-service");
$injector.require("platformProjectService", "./services/platform-service");

$injector.requireCommand("create", "./commands/create-project");
$injector.requireCommand("platform|*list", "./commands/list-platforms");
Expand Down
2 changes: 2 additions & 0 deletions lib/declarations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ interface INodePackageManager {
cache: string;
load(config?: any): IFuture<void>;
install(where: string, what: string): IFuture<any>;
Copy link
Contributor

Choose a reason for hiding this comment

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

This method can be made private in the implementation. The installSafe method can be renamed to just install, then.

tryExecuteAction(action: any, args?: any[]): IFuture<void>;
Copy link

Choose a reason for hiding this comment

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

Should this be part of the public interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now I don't need it in public interface, but I left it only for future usage

Copy link

Choose a reason for hiding this comment

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

Let's expose it when we need it.

downloadNpmPackage(packageName: string, pathToSave?: string): IFuture<string>;
}

interface IPropertiesParser {
Expand Down
14 changes: 12 additions & 2 deletions lib/definitions/platform.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,16 @@ interface IPlatformService {
buildPlatform(platform: string): IFuture<void>;
}

interface IPlatformCapabilities {
interface IPlatformData {
frameworkPackageName: string;
platformProjectService: IPlatformSpecificProjectService;
projectRoot: string;
normalizedPlatformName: string;
Copy link

Choose a reason for hiding this comment

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

Do we need the normalized prefix?

targetedOS?: string[];
}
}

interface IPlatformsData {
platformsNames: string[];
getPlatformData(platform: string): IPlatformData;
}

27 changes: 16 additions & 11 deletions lib/definitions/project.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,31 @@
interface IProjectService {
createProject(projectName: string, projectId: string): IFuture<void>;
createPlatformSpecificProject(platform: string): IFuture<void>;
prepareProject(normalizedPlatformName: string, platforms: string[]): IFuture<void>;
buildProject(platform: string): IFuture<void>;
ensureProject(): void;
projectData: IProjectData;
}

interface IPlatformProjectService {
createProject(projectData: IProjectData): IFuture<void>;
buildProject(projectData: IProjectData): IFuture<void>;
}

interface IProjectData {
projectDir: string;
projectName: string;
platformsDir: string;
projectFilePath: string;
projectId?: string;
projectName?: string;
}

interface IProjectTemplatesService {
defaultTemplatePath: IFuture<string>;
installAndroidFramework(whereToInstall: string): IFuture<string>
}

interface IPlatformProjectService {
createProject(platform: string): IFuture<void>;
buildProject(platform: string): IFuture<void>;
prepareProject(normalizedPlatformName: string, platforms: string[]): IFuture<void>;
}

interface IPlatformSpecificProjectService {
validate(): void;
checkRequirements(): IFuture<void>;
createProject(projectRoot: string, frameworkDir: string): IFuture<void>;
interpolateData(projectRoot: string): void;
executePlatformSpecificAction(projectRoot: string, frameworkDir: string): void;
buildProject(projectRoot: string): IFuture<void>;
}
31 changes: 31 additions & 0 deletions lib/node-package-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,14 @@
import npm = require("npm");
import Future = require("fibers/future");
import shell = require("shelljs");
import path = require("path");

export class NodePackageManager implements INodePackageManager {
private static NPM_LOAD_FAILED = "Failed to retrieve data from npm. Please try again a little bit later.";

constructor(private $logger: ILogger,
private $errors: IErrors) { }

public get cache(): string {
return npm.cache;
}
Expand Down Expand Up @@ -32,5 +38,30 @@ export class NodePackageManager implements INodePackageManager {
});
return future;
}

public tryExecuteAction(action: any, args?: any[]): IFuture<void> {
Copy link
Contributor

Choose a reason for hiding this comment

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

action shouldn't be any. Rather, it should be, let's say, action: (...args: any[]) => void.

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 simplify the signature for args by using open-ended arguments: ...args: any[]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree :) It looks better

return (() => {
try {
this.load().wait(); // It's obligatory to execute load before whatever npm function
action(args);
Copy link
Contributor

Choose a reason for hiding this comment

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

Most likely you need to use apply here. action.apply(null, args).

} catch(error) {
this.$logger.debug(error);
this.$errors.fail(NodePackageManager.NPM_LOAD_FAILED);
}
}).future<void>()();
}

public downloadNpmPackage(packageName: string, pathToSave?: string): IFuture<string> {
Copy link

Choose a reason for hiding this comment

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

This is actually doing an install, not just download? Why hot rename it to something like installSafe.

return (() => {
var action = (packageName: string) => {
this.install(pathToSave || npm.cache, packageName).wait();
};

this.tryExecuteAction(action, [packageName]).wait();
Copy link
Contributor

Choose a reason for hiding this comment

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

With open-ended arguments this becomes (action, packageName)


return path.join(pathToSave || npm.cache, "node_modules", packageName);

}).future<string>()();
}
}
$injector.register("npm", NodePackageManager);
78 changes: 43 additions & 35 deletions lib/services/platform-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,25 +4,45 @@ import path = require("path");
import util = require("util");
import helpers = require("./../common/helpers");

export class PlatformService implements IPlatformService {
private platformCapabilities: { [key: string]: IPlatformCapabilities } = {
class PlatformsData implements IPlatformsData {
private platformsData: { [key: string]: IPlatformData } = {
Copy link

Choose a reason for hiding this comment

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

You can omit the signature [key: string]: IPlatformData and it will be inferred. This will allow you to change your other code to: this.platformsData.ios.projectRoot = "" instead of this.platformsData["ios"].projectRoot = "";

ios: {
frameworkPackageName: "tns-ios",
platformProjectService: $injector.resolve("iOSProjectService"),
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 poor practice to use the global $injector from a class. Rather, get the injector as a constructor dependency. this.$injector.resolve(...).

Better yet, don't use explicit resolution at all, take the two project services as constructor dependencies.

Copy link

Choose a reason for hiding this comment

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

What about lazy loading the project services? Why do we need to load the Android one if we are just doing stuff for iOS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we discussed, we need to modify yok to support lazy loading. For now I'll inject the two project services as constructor dependencies and in another PR I'm going to implement lazy loading. Agree?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure. Just add it as a task in the story.

normalizedPlatformName: "iOS",
projectRoot: "",
Copy link

Choose a reason for hiding this comment

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

Do we need to set this, because it is already done in the constructor?

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, we need it, because projectRoot is declared in IPlatformData interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is set in constructor because here I can't use projectData

Copy link

Choose a reason for hiding this comment

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

When you remove the type I think you can safely omit setting it here and leave it only in the constructor.

targetedOS: ['darwin']
},
android: {
frameworkPackageName: "tns-android",
platformProjectService: $injector.resolve("androidProjectService"),
normalizedPlatformName: "Android",
projectRoot: ""
}
};

private platformNames = [];
constructor($projectData: IProjectData) {
this.platformsData["ios"].projectRoot = "";
this.platformsData["android"].projectRoot = path.join($projectData.platformsDir, "android");
}

constructor(private $errors: IErrors,
private $fs: IFileSystem,
private $projectService: IProjectService) {
this.platformNames = Object.keys(this.platformCapabilities);
public get platformsNames() {
return Object.keys(this.platformsData);
}

public getCapabilities(platform: string): IPlatformCapabilities {
return this.platformCapabilities[platform];
public getPlatformData(platform): IPlatformData {
return this.platformsData[platform];
}
}
$injector.register("platformsData", PlatformsData);

export class PlatformService implements IPlatformService {
constructor(private $errors: IErrors,
private $fs: IFileSystem,
private $projectService: IProjectService,
private $platformProjectService: IPlatformProjectService,
private $projectData: IProjectData,
private $platformsData: IPlatformsData) {
}

public addPlatforms(platforms: string[]): IFuture<void> {
Expand All @@ -33,7 +53,7 @@ export class PlatformService implements IPlatformService {

this.$projectService.ensureProject();

var platformsDir = this.$projectService.projectData.platformsDir;
var platformsDir = this.$projectData.platformsDir;
this.$fs.ensureDirectoryExists(platformsDir).wait();

_.each(platforms, platform => {
Expand All @@ -49,7 +69,7 @@ export class PlatformService implements IPlatformService {

this.validatePlatform(platform);

var platformPath = path.join(this.$projectService.projectData.platformsDir, platform);
var platformPath = path.join(this.$projectData.platformsDir, platform);

// TODO: Check for version compatability if the platform is in format platform@version. This should be done in PR for semanting versioning

Expand All @@ -58,26 +78,26 @@ export class PlatformService implements IPlatformService {
}

// Copy platform specific files in platforms dir
this.$projectService.createPlatformSpecificProject(platform).wait();
this.$platformProjectService.createProject(platform).wait();

}).future<void>()();
}

public getInstalledPlatforms(): IFuture<string[]> {
return(() => {
if(!this.$fs.exists(this.$projectService.projectData.platformsDir).wait()) {
if(!this.$fs.exists(this.$projectData.platformsDir).wait()) {
return [];
}

var subDirs = this.$fs.readDirectory(this.$projectService.projectData.platformsDir).wait();
return _.filter(subDirs, p => { return this.platformNames.indexOf(p) > -1; });
var subDirs = this.$fs.readDirectory(this.$projectData.platformsDir).wait();
return _.filter(subDirs, p => { return this.$platformsData.platformsNames.indexOf(p) > -1; });
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 the shorter C#-style lambda form: p => this.$platformsData.platformsNames.indexOf(p) > -1

}).future<string[]>()();
}

public getAvailablePlatforms(): IFuture<string[]> {
return (() => {
var installedPlatforms = this.getInstalledPlatforms().wait();
return _.filter(this.platformNames, p => {
return _.filter(this.$platformsData.platformsNames, p => {
return installedPlatforms.indexOf(p) < 0 && this.isPlatformSupportedForOS(p); // Only those not already installed
});
}).future<string[]>()();
Expand All @@ -93,9 +113,9 @@ export class PlatformService implements IPlatformService {
return (() => {
platform = platform.toLowerCase();
this.validatePlatform(platform);
var normalizedPlatformName = this.normalizePlatformName(platform);

this.$projectService.prepareProject(normalizedPlatformName, this.platformNames).wait();
this.$platformProjectService.prepareProject(this.$platformsData.getPlatformData(platform).normalizedPlatformName,
this.$platformsData.platformsNames).wait();
}).future<void>()();
}

Expand All @@ -104,13 +124,13 @@ export class PlatformService implements IPlatformService {
platform = platform.toLocaleLowerCase();
this.validatePlatform(platform);

this.$projectService.buildProject(platform).wait();
this.$platformProjectService.buildProject(platform).wait();
}).future<void>()();
}

private validatePlatform(platform: string): void {
if (!this.isValidPlatform(platform)) {
this.$errors.fail("Invalid platform %s. Valid platforms are %s.", platform, helpers.formatListOfNames(this.platformNames));
this.$errors.fail("Invalid platform %s. Valid platforms are %s.", platform, helpers.formatListOfNames(this.$platformsData.platformsNames));
}

if (!this.isPlatformSupportedForOS(platform)) {
Expand All @@ -119,29 +139,17 @@ export class PlatformService implements IPlatformService {
}

private isValidPlatform(platform: string) {
return this.platformCapabilities[platform];
return this.$platformsData.getPlatformData(platform);
}

private isPlatformSupportedForOS(platform: string): boolean {
var platformCapabilities = this.getCapabilities(platform);
var targetedOS = platformCapabilities.targetedOS;
var targetedOS = this.$platformsData.getPlatformData(platform).targetedOS;

if(!targetedOS || targetedOS.indexOf("*") >= 0 || targetedOS.indexOf(process.platform) >= 0) {
return true;
}

return false;
}

private normalizePlatformName(platform: string): string {
switch(platform) {
case "android":
return "Android";
case "ios":
return "iOS";
}

return undefined;
}
}
$injector.register("platformService", PlatformService);
$injector.register("platformService", PlatformService);
Loading