install plugin with npm api

[nurra01]

This will help to install and update plugins without user's npm, so user is not dependent to have npm.

[ghost]

So is there any reason why you didn't just make npm a dependency and then do execSync('./node_modules/npm/bin/npm command'). That seems like it would be way easier to me.

EDIT: I did a quick hack of what I am talking about. Would something like this be possible?

// This could definitely be cleaner but you get what I am talking about.
// Just saying I would be more comfortable with this since the APIs you are using aren't supported.
const execOutput = execSync(`node "${path.resolve(__dirname, "../../../../../../node_modules/npm")}" install "${npmPackage}" --prefix "${PMFConstants.instance.PLUGIN_INSTALL_LOCATION}" ` +
        `-g --registry "${registry}"`, {
        cwd: PMFConstants.instance.PMF_ROOT,
        stdio: pipe
});

Also keytar is not an optional dependency, it must be a devDependency so please change this.

And please check this against defect #67 to verify that there aren't any problems.

[nurra01]

@AHumanFromCA I think this is not going to work, because user won't have node.js, so we cannot runt script above. I will recheck it.

[ghost]

It was just a quick hack, you could see if you can just run the command from node_modules outright too.

[plavjanik]

The reason why we do not call node by execSync() is that it will not work well when we use zeit/pkg to package the CLI into a standalone executable bundle. There does need to be any Node installed on the system and if it is installed it is a different Node than the one inside the bundle.

[ghost]

@plavjanik I realize that node may or may not be installed globally, but is there any way you could access the node runtime provided in a binary. I'm just not comfortable with using the npm apis without proper documentation about how to use them or indication of support for them.

Regardless, keytar needs to be moved from optionalDependencies to devDependencies for me to dismiss my request changes

Keytar has been moved back to devDependencies. Someone else more comfortable with the decision should approve this.

[ghost]

It looks like this change also broke the existing plugin management facility integration tests. From my understanding, this change should be transparent to the CLI interface, so please fix without modifying the tests if at all possible.

[plavjanik]

Yes, the existing tests need to pass. We are concerned about the stability of the NPM API. We include a
specific version into the bundled version, so there we can be sure that expected NPM API is there. For the NPM-installed version, we can use the NPM CLI.

[plavjanik]

Yes, the existing tests need to pass. We are concerned about the stability of the NPM API. We include a
specific version into the bundled version, so there we can be sure that expected NPM API is there. For the NPM-installed version, we can use the NPM CLI.

[plavjanik]

Yes, the existing tests need to pass. We are concerned about the stability of the NPM API. We include a
specific version into the bundled version, so there we can be sure that expected NPM API is there. For the NPM-installed version, we can use the NPM CLI.

[plavjanik]

Yes, the existing tests need to pass. We are concerned about the stability of the NPM API. We include a
specific version into the bundled version, so there we can be sure that expected NPM API is there. For the NPM-installed version, we can use the NPM CLI.

[plavjanik]

Yes, the existing tests need to pass. We are concerned about the stability of the NPM API. We include a
specific version into the bundled version, so there we can be sure that expected NPM API is there. For the NPM-installed version, we can use the NPM CLI.

[plavjanik]

Yes, the existing tests need to pass. We are concerned about the stability of the NPM API. We include a
specific version into the bundled version, so there we can be sure that expected NPM API is there. For the NPM-installed version, we can use the NPM CLI.

[plavjanik]

Yes, the existing tests need to pass. We are concerned about the stability of the NPM API. We include a
specific version into the bundled version, so there we can be sure that expected NPM API is there. For the NPM-installed version, we can use the NPM CLI.

[plavjanik]

Yes, the existing tests need to pass. We are concerned about the stability of the NPM API. We include a
specific version into the bundled version, so there we can be sure that expected NPM API is there. For the NPM-installed version, we can use the NPM CLI.

[plavjanik]

Yes, the existing tests need to pass. We are concerned about the stability of the NPM API. We include a
specific version into the bundled version, so there we can be sure that expected NPM API is there. For the NPM-installed version, we can use the NPM CLI.

[plavjanik]

There is a problem with GitHub. It says that it cannot send a comment but it appears there several minutes ago.

[ghost]

Hi @plavjanik, I can confirm that I see your comment at least 7 times on this PR lol. Darn GH having issues.

[ghost]

Please install packages as a global module. That will get rid of these warnings:

npm WARN saveError ENOENT: no such file or directory, open 'C:\dev\.zowe\plugins\installed\package.json'
npm notice created a lockfile as package-lock.json. You should commit this file.
npm WARN enoent ENOENT: no such file or directory, open 'C:\dev\.zowe\plugins\installed\package.json'

I suspect this is also why it does not work on linux

[ghost]

Expanding on my second rejection a bit:

I have 2 main issues with the messages you see here.

• A package.lock file was created under the plugins/installed folder. This could have negative implications based on how that file is used by npm.
• Based on it complaining that there isn't a package.json, it looks like npm is being tricked into thinking plugins/installed is a node_module when it really isn't.

So I can tell that we are no longer installing globally like the cli did with the -g and --prefix tags. This leads to the following problems:

• The file structure of the directory has changed and will cause problems on some systems.
• Anyone upgrading from an imperative based cli to this new version of imperative will have to delete their plugins directory and reinstall all their plugins again.
• Npm is tricked into thinking that the directory is a npm package when it really isn't. Since this is being used as a global install, it should be treated as such.

That being said, the only acceptable answer is to have the API still install as a global module.

Additional Comments:

I still have reservations about using undocumented APIS so I am not comfortable being the approver (I will delegate that to another team member if necessary). I would be much more comfortable if you could provide a public link by npm that provides documentation on how to use their APIs.

Also I think that including npm itself as part of imperative a good thing because it

• Makes imperative independent of a globally installed npm version.
• Allows for any imperative based cli to be packed up into an executable and placed onto a machine that does not have npm installed.

[nurra01]

@AHumanFromCA we couldn't find official documentation how to use NPM APIs, it seems it just doesn't exist. So we needed to look at their code and use functions from there. There are other NPM packages which are solving what we want to do, but they are using the same NPM functions in their code, so it doesn't make sense to use external package.
Link for one of them is https://github.com/maxleiko/npmi
Link for NPM github: https://github.com/npm/cli

[tucker01]

Hi All, how much work do we think is left until we approve this PR? If it's a significant amount, can we close and re-open? We'd like to get #83 merged ASAP (another story depends on the changes for completion) and we don't normally skip line (unless you guys are OK with leaving it open and allowing subsequent PRs to merge).

[nurra01]

@tucker01 Hi, last commit should solve requests mentioned above so in my opinion there is no work left. Could you guys to review and test it today? Thanks

[tucker01]

Imperative Jenkins runs the full test suite so if the checks pass, we should be good... now just need @AHumanFromCA approval for requested changes.

Thanks!

[ghost]

I still see this message when installing plugins.

npm WARN enoent ENOENT: no such file or directory, open 'C:\dev\.zowe\plugins\installed\package.json'

Looks like we removed 2 of the 3 messages but this one still concerns me. Could you figure out how to also remove this message?

Also, I do appreciate you giving some links to documentation. It gives me a better picture where you got your information from. Could you do one additional thing as well? Where you do your npm.load and npm.commands.install, can you add some documentation about why this order is necessary, what parameters it expects, and describe a little bit why those npm.config.sets are necessary. I think this will help out with supportability in the long run.

Thanks

[nurra01]

@AHumanFromCA I cannot figure out how to remove this message, because its NPM behavior to search for a package.json by default. If you try to run npm.commands.install or npm.config.set/get commands outside of npm.load it will throw an error that commands need to be inside of npm.load. Another optios to see what arguments it needs you can just click npm in the code, it should open you index.d.ts file where all arguments are described. Also there is a link: https://stackoverflow.com/questions/15957529/can-i-install-a-npm-package-from-javascript-running-in-node-js/15957574#15957574
I used npm.config stuff to manage prefix and global options, actually npm.load should get all these options and use them, but it is not doing that, so I needed to change configs. Also, if you look at npm-cli.js inside of node_modules/npm/bin folder you cann see how they use npm.load.

[ghost]

Hi @nurra01,

If you cannot get rid of that message then we need to document it in the output of the install command somehow. (If you already did then great!)

And I still would like to see some of that description documented in the code so that someone understands this when reading the code. Someone new could come in and have a better idea on the why it is written in that order and how it works without having to do all the searches you did. You could even include the stackoverflow link in the comments as well.

Thanks

[ghost]

Thank you for the comments in the install doc. Those will help future developers have more to go on if support is needed in this area.

In regards to document the warning:

Currently when installing a plugin you will see this text (using from zowe-cli as my example):

Imperative's plugin installation program handles peer dependencies for modules
in the @brightside namespace, so you can safely ignore NPM warnings about
missing peer dependencies related to @brightside modules.

and these warnings

npm WARN enoent ENOENT: no such file or directory, open 'C:\dev\.zowe\plugins\installed\package.json'
npm WARN @brightside/[email protected] requires a peer of @brightside/[email protected] but none is installed. You must install peer dependencies yourself.
npm WARN @brightside/[email protected] requires a peer of @brightside/[email protected] but none is installed. You must install peer dependencies yourself.

That text should be altered to also include ignoring the no such file or directory for your package.json path.

Maybe it could read

Imperative's plugin installation program handles peer dependencies for modules in the @brightside namespace, so you can safely ignore NPM warnings about missing peer dependencies related to @brightside modules. The install program also handles NPM warnings about a missing path-to-plugin-directory/package.json

In my case path-to-plugin-directory would be C:\dev\.zowe\plugins\installed, but yours may differ.

Perhaps @BrandonJenkins14 or @JamesBauman could help out with the wording

[ghost]

Tested again today and saw this message now:

Imperative's plugin installation program handles peer dependencies for modules
in the @brightside namespace and missing package.json file,
so you can safely ignore NPM warnings about
missing peer dependencies related to @brightside modules and absent C:\dev\.zowe\plugins\installedpackage.json file.

you should do this: path.join(PMFConstants.instance.PLUGIN_INSTALL_LOCATION, "package.json") so that the path is formatted correctly.

I did talk with @BrandonJenkins14 today and he says that the wording is good so no changes there. So make this change and we should be good to merge this. I still have some reservations about the approach, like how often might we have to update our npm dependency to maintain compatibility with the registry, but your comments at least make it easy to get up to speed with how the code works.

On a side note, we should probably be using TextUtils.wordWrap instead of manually making the line wraps. I will open a separate issue for this.

[nurra01]

Thank you for suggestions, last commit should fix that.