Skip to content

Implement install command #589

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
merged 1 commit into from
Jun 25, 2015
Merged

Implement install command #589

merged 1 commit into from
Jun 25, 2015

Conversation

Fatme
Copy link
Contributor

@Fatme Fatme commented Jun 24, 2015

@Fatme Fatme added this to the 1.1.2 milestone Jun 24, 2015
@Fatme Fatme added the feature label Jun 24, 2015
@ns-bot
Copy link

ns-bot commented Jun 24, 2015

✅

rosen-vladimirov
rosen-vladimirov reviewed Jun 24, 2015
View reviewed changes
lib/commands/install.ts

execute(args: string[]): IFuture<void> {
return (() => {
let projectFilePath = this.getProjectFilePath(args[0]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you can cache the result of this.getProjectFilePath(args[0]) in a variable, as you have already read the file and its content in canExecute.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe an even better solution is to cache the contents of package.json (or the nativescript key) inside getProjectData.

@rosen-vladimirov
Copy link
Contributor

why is bin/nativescript.js changed:100644 → 100755 ?

rosen-vladimirov
rosen-vladimirov reviewed Jun 24, 2015
View reviewed changes
lib/commands/install.ts
let platformData = $platformsData.getPlatformData(platform);
let frameworkPackageData = projectData[platformData.frameworkPackageName];
if(frameworkPackageData && frameworkPackageData.version) {
this.$injector.resolve("platformService").addPlatforms([`${platform}@${frameworkPackageData.version}`]).wait();
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 resolve platformService outside of each statement

@rosen-vladimirov
Copy link
Contributor

I'm unable to use this command, I've tried the following scenarios:
directory app1003 with package.json in it

NOTE: In my app1003 direcotry I have only package.json file. My init directory is empty.

  • in another directory (called init), I call $ tns install ../app1003 - this fails with Error: No project found at or above 'd:\Work\nativescript-cli\scratch\app1003\app1002' and neither was a --path specified
  • inside app1003, I created init directory and from it I've tried $ tns install ../ - this passes, but the folder structure is:
$ pwd
/d/Work/nativescript-cli/scratch/app1003

vladimirov@VLADIMIROV /d/Work/nativescript-cli/scratch/app1003 (fatme/install-command)
$ ls -ltr
total 1
-rw-r--r--    1 vladimir Administ      106 Jun 24 11:21 package.json
drwxr-xr-x    2 vladimir Administ        0 Jun 24 11:42 init
drwxr-xr-x    3 vladimir Administ        0 Jun 24 11:42 app1002
drwxr-xr-x    3 vladimir Administ        0 Jun 24 11:42 platforms
drwxr-xr-x    2 vladimir Administ        0 Jun 24 11:42 node_modules

vladimirov@VLADIMIROV /d/Work/nativescript-cli/scratch/app1003 (fatme/install-command)
$ ls -l init
total 0
  • I've tried tns install directly in the app1003 directory. This fails with TypeError: Arguments to path.resolve must be strings
  • I've tried tns install . directly in the app1003 directory. This passes, but the directory structure after that is:
$ pwd
/d/Work/nativescript-cli/scratch/app1003

vladimirov@VLADIMIROV /d/Work/nativescript-cli/scratch/app1003 (fatme/install-command)
$ ls -l
total 1
drwxr-xr-x    3 vladimir Administ        0 Jun 24 11:44 app1002
drwxr-xr-x    2 vladimir Administ        0 Jun 24 11:44 node_modules
-rw-r--r--    1 vladimir Administ      106 Jun 24 11:21 package.json
drwxr-xr-x    3 vladimir Administ        0 Jun 24 11:44 platforms

vladimirov@VLADIMIROV /d/Work/nativescript-cli/scratch/app1003 (fatme/install-command)
$ ls -l app1002
total 0
drwxr-xr-x    5 vladimir Administ        0 Jun 24 11:44 app1002

vladimirov@VLADIMIROV /d/Work/nativescript-cli/scratch/app1003 (fatme/install-command)
$ ls -l app1002/app1002
total 3
drwxr-xr-x   11 vladimir Administ     4096 Jun 24 11:44 app
-rw-r--r--    1 vladimir Administ      106 Jun 24 11:44 package.json
drwxr-xr-x    2 vladimir Administ        0 Jun 24 11:44 platforms

NOTE: My package.json is:

$ cat package.json
{
        "nativescript": {
                "id": "org.nativescript.app1002",
                "tns-android": {
                        "version": "1.1.0"
                }
        }
}

@Fatme Fatme force-pushed the fatme/install-command branch from 7576a5c to e6abdc7 Compare June 24, 2015 09:18
@ns-bot
Copy link

ns-bot commented Jun 24, 2015

✅

@Fatme
Copy link
Contributor Author

Fatme commented Jun 24, 2015

@rosen-vladimirov
why is bin/nativescript.js changed:100644 → 100755 - I did it because I get permission denied every time I tried to execute tns command
I've fixed the command behavior in scenarios you mention.

rosen-vladimirov
rosen-vladimirov reviewed Jun 24, 2015
View reviewed changes
lib/commands/install.ts

private getProjectData(projectFilePath: string): IFuture<any> {
return (() => {
let fileContent = this.$fs.readJson(projectFilePath).wait();
Copy link
Contributor

Choose a reason for hiding this comment

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

as @teobugslayer pointed, it will be great if you can cache the projectData as well

@Fatme Fatme force-pushed the fatme/install-command branch from e6abdc7 to 28ddc2f Compare June 24, 2015 16:21
@ns-bot
Copy link

ns-bot commented Jun 24, 2015

✅

@rosen-vladimirov
Copy link
Contributor

👍 after green build
but we'll need help file for the new command :)

Fatme pushed a commit that referenced this pull request Jun 25, 2015
Merge pull request #589 from NativeScript/fatme/install-command
78df478 
Implement install command
@Fatme Fatme merged commit 78df478 into release Jun 25, 2015
@Fatme Fatme deleted the fatme/install-command branch June 25, 2015 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
1.1.2
Development

Successfully merging this pull request may close these issues.
4 participants
@Fatme @ns-bot @rosen-vladimirov @teobugslayer