Skip to content

parse npm install results correctly #3213

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.

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 1 commit into from
Nov 15, 2017

Conversation

dnadoba
Copy link
Contributor

@dnadoba dnadoba commented Nov 10, 2017

Solves Issue #3217

This pull request changes the implementation to search for the correct object by name. It does no longer depend on the position and should be more reliable especially as the --json flag is experimental as described in the npm documentation: https://docs.npmjs.com/misc/config#json

As described in the comment of the current implementation of parseNpmInstallResults we just use the first object of the updated array.

// Npm 5 return different object after performing `npm install --dry-run`.
// Considering that the dependency is already installed we should
// find it in the `updated` key as a first element of the array.

But when I execute the following command:

npm install tns-ios --json --dry-run

tns-ios is not the first element in the updated array. The output looks something like this:

{
  "added": [],
  "removed": [],
  "updated": [
    {
      "action": "update",
      "name": "colors",
      "version": "https://registry.npmjs.org/colors/-/colors-1.1.2.tgz",
      "path": "<path to project>/node_modules/colors",
      "previousVersion": ""
    },
    {
      "action": "update",
      "name": "inherits",
      "version": "https://registry.npmjs.org/inherits/-/inherits-2.0.3.tgz",
      "path": "<path to project>/node_modules/inherits",
      "previousVersion": ""
    },
    {
      "action": "update",
      "name": "tns-ios",
      "version": "3.3.0",
      "path": "<path to project>/node_modules/tns-ios",
      "previousVersion": "3.3.0"
    }
  ],
  "moved": [],
  "failed": [],
  "elapsed": 3113
}

Instead of the first element, tns-ios is the last element.

I had an issue with the following command:

tns platform add ios

It always failed with the following error:

cp: no such file or directory: <path to project>/node_modules/<some random updated module>/framework/*

Now it works like a charm.

@ns-bot
Copy link

ns-bot commented Nov 10, 2017

Can one of the admins verify this patch?

@rosen-vladimirov
Copy link
Contributor

run ci

@rosen-vladimirov
Copy link
Contributor

Hey @dnadoba ,
Thank you very much for your contribution! We are in process of verifying the change, but meanwhile can you please specify the versions of Node.js and npm that you have used on your side (the ones which produce the output described in your PR):

$ node -v 
$ npm -v

@dtopuzov
Copy link
Contributor

run ci

@dnadoba
Copy link
Contributor Author

dnadoba commented Nov 13, 2017

@rosen-vladimirov
Node: v8.9.0
NPM: 5.5.1

@rosen-vladimirov
Copy link
Contributor

run ci

@rosen-vladimirov
Copy link
Contributor

Hey @dnadoba ,
The code looks great and the only failing test was due to infrastructure issues on our side. I've rerun the tests, but meanwhile, can you please send us some steps to reproduce the issue? I've tried creating a new project on macOS by using your Node.js and npm versions, but I was unable to reproduce the problem.

@rosen-vladimirov
Copy link
Contributor

Hey @buuhuu ,
Can you please share your steps to reproduce as well? It will be a great help in case you can send such steps, so we'll be able to add an automated test on our side. I'm still not able to reproduce the issue, but I'm sure I'm missing something here.

@dnadoba
Copy link
Contributor Author

dnadoba commented Nov 14, 2017

It has something to do with my dependencies and the package-lock.json.
I could not create a simple sample app because I just getting started with NativeScript. (This was basically my first day with NativeScript 😅).
The app I work on comes from a company I work for.
I just get used to the Project and cannot work any longer on that issue.
If you want, I can give you the package.json and the package-lock.json.

@rosen-vladimirov
Copy link
Contributor

Hey @dnadoba ,
Woow, first day and great contribution, I can't wait to see what will happen when you get used to NativeScript 😄
If you can send both files (in case they do not contain any private information), this will be a great help for us. Thanks again for your cooperation!

Copy link
Contributor

@rosen-vladimirov rosen-vladimirov left a comment

Choose a reason for hiding this comment

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

Code looks great. We'll do some manual verifications and we'll merge this PR.

@dnadoba
Copy link
Contributor Author

dnadoba commented Nov 14, 2017

I created a gist containing both files(sensible information removed):
https://gist.github.com/dnadoba/0cf3590eb06fc34c5c5f5c7ebb6a1ebf
Let me know if you can reproduce the issue or if something is missing.

@rosen-vladimirov
Copy link
Contributor

Hey @dnadoba ,
Thanks for sharing the information. We've successfully reproduced the issue on our side and verified your fix is correct. I'll merge this PR and also we'll include it in the incoming 3.3.1 release of NativeScript CLI.
Congratulations on this quality PR! Have a badge!
contrib

@rosen-vladimirov rosen-vladimirov merged commit 66c21c2 into NativeScript:master Nov 15, 2017
@dtopuzov dtopuzov added this to the 3.4.0 milestone Nov 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants