[Feature request] Run default npm scripts from Task Runner

[BrainCrumbz]

Installed product versions

• Visual Studio: 2015 Community ed.
• This extension: 1.1.44

  Description

Thinking back, your comment on issue #19 got us thinking:

Great idea. What do we do if there is a postinstall script but no install? Do we create the install parent node in that case since it has to be executable for the postinstall to work?

At the moment there's no way to run default npm commands from the Task Runner, unless they are actually listed. So e.g. we have defined start in our scripts, and we can run it, but e.g. we did not define install, but still we can run that from console.

Maybe it could be worth it to add them to the Runner as well. They'd be some kind of ghost items, or implicit items, that are listed even if not actually present.

In this scenario, your earlier comment comes back to place: if scripts contains postinstall, that would be nested under the implicit install.

Does that make sense? (For this same release, in case?)

[BrainCrumbz]

This would be the full list of default commands: https://github.com/npm/npm/blob/master/lib/run-script.js#L64 , to be executed as npm <command> (no run needed of course)

[madskristensen]

It seems like version and install are the only two default commands that can always be executed without being added to the scripts section in package.json. Should we always add those two to the task list?

[madskristensen]

Actually, version can't be executed the same way. How about just adding install as a permanent task in the list?

[BrainCrumbz]

Uhm, install and uninstall for sure can be run without being listed. One can add custom pre / post commands for those. Don't remember if one can actually declare its own install or uninstall.

We'll do some tests on those and on other default commands.

[madskristensen]

I've updated the extension with auto-injection of install and uninstall nodes if any post- or -pre command is defined. The new parent install and uninstall nodes cannot be executed - they are used for nesting only. Check it out here http://vsixgallery.com/extension/d7f89ba3-815c-4feb-89b9-68d1654e2138/

[scottaddie]

It is possible to declare your own install script in package.json. Here are a few examples:

1. https://github.com/scottaddie/webpack-angular-es2015/blob/master/package.json#L13
2. https://github.com/johnpapa/gulp-patterns/blob/master/package.json#L16

[madskristensen]

So if you can declare your own install, then what should be the behavior here?

Here's a suggestion:

1. The install task node is always created when preinstall or postinstall is present
2. The install task runs npm run install when clicked

What about uninstall?

[scottaddie]

@madskristensen I agree with number 2 listed above. As for number 1 in your comment, I would expect install to display all the time. If I define my own commands in the install script, the npm packages will still be installed. My commands should execute immediately after.

[scottaddie]

The uninstall command, in my opinion, is another great task to display at all times. It's a nice alternative to hopping out to the command shell and running rimraf.

[scottaddie]

It would also be nice if, when right-clicking the uninstall task, you see a list of top-level modules installed locally that can be uninstalled. For example, run npm list --depth=0 at the same level as the package.json file. I would expect to see those packages listed when right-clicking the uninstall task. You would ideally have the ability to select multiple modules from the context menu which appears.

[BrainCrumbz]

Sorry for being quiet :smile: , trying to recap my thoughts here:

1. as a general rule, npm task runner explorer (TRX) should "follow" the same rationale offered by npm when running scripts. This is so that existing npm users are not "surprised", and so that newcomers can "learn" its usage the right way and can easily move between VS and console.
2. among default (so to say) scripts commands, all those that can be always run, whether they're in package.json or not, should be always listed. This is so that user can always run them from TRX. This would be the case e.g. for install and uninstall.
3. If there are default commands that can be run only if listed in package.json (don't know if those exist), then those should appear only if found in json.
4. If previous case is actually possible, that would leave an open point on what to do when the command is not found, but there's a post- pre- related command. Although this seems a little counter intuitive ... so I'd first check if this can happen at all. Anyway, I'd stick mimicking actual npm behaviour.
5. as already suggested by @scottaddie , npm TRX could show commands under either one of two "subtrees": Default and Custom. That naming seems quite bound/close to what they actually contains.

[BrainCrumbz]

After some quick tests, among default scripts:

1. The following can always be run, whether they're actually present in package.json or not:
   • install
   • uninstall
   • test
   • publish
   • restart
   • version, although as @madskristensen mentioned this cannot be launched effectively from TRX, because it would also need an additional <newversion> parameter. In this case, one should decide where to show a pre- or postversion command if found.
   • For each of those, if a command with same name is found, npm will execute that as well
   • For each of those, if a pre- or post- command is found, npm will execute that as well. So npm TRX should nest those under related default node.
2. The following can be launched only if actually listed in package.json:
   • start
   • stop
   • Hence, npm TRX should list those under Default group only if found.
   • If they're not found, and some prestart, prestop, ... is found, one should decide what to do with them in npm TRX. Should they be listed as ordinary Custom commands? Or maybe listed among Default, under a "disabled" start/stop node.

[scottaddie]

@BrainCrumbz In response to the 2nd item in your response immediately above, I would expect to see a "disabled" start / stop node. This would serve as a hint to the developer that a start and/or stop hook exists and can be defined in the package.json file.

[BrainCrumbz]

Totally agree. Sticking with that proposal, it remains yet to decide what to do with pre / postversion commands if found, as at the moment there's no way to effectively run version from npm TRX.

EDIT staying consistent, one way could be:

1. if no pre nor postversion is found, do nothing
2. if pre and/or postversion is found, show a "disabled" version command among Default, and nest them underneath, disabled as well.

Basically, even if they're found, they cannot be ran from TRX.

[madskristensen]

I've just made the modifications discussed here.

Consider this package.json file:

{
    "dependencies": {
        "knockout": "3.4.0"
    },
    "devDependencies": {
        "jquery": "2.2.2"
    },
    "scripts": {
        "foo": "echo foo",
        "prefoo": "echo foo",
        "install": "echo install",
        "postinstall": "echo postinstall",
        "preinstall": "echo preinstall",
        "posttest": "echo post test",
        "prestart": "",
        "preuninstall": "echo preuninstall",
        "postuninstall": "echo postuninstall",
        "preversion": "echo preversion"
    }
}

It will produce this in TRX:

version is created because perversion exist, but it is readonly (TRX makes readonly nodes bold).

The CI build is here http://vsixgallery.com/extension/d7f89ba3-815c-4feb-89b9-68d1654e2138/ ready for testing

[BrainCrumbz]

Cool, I'll install that now. While looking at former commit, shouldn't test be moved from DEFAULT_TASKS to ALWAYS_TASKS? Thanks!

[madskristensen]

test was shown always, but since it doesn't do anything, I thought it just added noise to always show it

[BrainCrumbz]

I see.

Seems legit

[scottaddie]

@madskristensen I installed the latest version of the extension. One question for you. When binding the uninstallation of a specific npm module to a Visual Studio event, I see this:

Does it make sense to prepend the package name (babel-polyfill) appearing under the Before Build/package.json node with something like "uninstall: "? Displaying the package name by itself doesn't make it clear that uninstall will be invoked.

[madskristensen]

@scottaddie That's a good idea

[scottaddie]

@madskristensen If the npm modules haven't yet been installed in the project, it would be nice to disable the uninstall node in the extension. Thoughts? It could be as simple as checking for the presence of the node_modules folder.

Also, if I bind the uninstallation of all dependencies to an event, it may be beneficial to prepend "uninstall" to that as well. Here's what currently displays in the bindings, which may seem confusing:

[BrainCrumbz]

@scottaddie On this last one, npm TRX could also stick with what npm does out of the box: you're allowed to uninstall something, worst case if that's not installed you just get some error explaining situation. Just thinking...

[scottaddie]

@BrainCrumbz Honestly, I could go either way with this one. I definitely see your point.

@madskristensen While you're in there, I also believe that the string array declared here shouldn't contain bundleDependencies, bundledDependencies, or peerDependencies. The documentation for uninstall only mentions CLI flags for dependencies, devDependencies, and optionalDependencies.

[madskristensen]

I tried uninstalling a package specified in peerDependencies and it failed, so I've removed the peerDependencies and bundleDependencies as suggested.

Let's keep the uninstall node even if node_modules folder is missing. The TRX will not reload the task after an install, only when package.json has been modified, so it will have false negatives if we hide it

[scottaddie]

@madskristensen Here's one other potential problem I noticed. The user shouldn't be allowed to bind, for example, the entire dependencies node and 1 of its children (e.g., babel-polyfill) to the same Visual Studio event. Here's a screenshot to clarify:

Since dependencies is already attached to the binding, babel-polyfill would be accounted for in the uninstall which runs during the "Before Build" event. How should this be handled?

[madskristensen]

There is no API in TRX to handle that easily. I think this represents an edge case only and I'm not convinced that we should spend time coming up with a fix at this point in time

[BrainCrumbz]

Only throwing our 2c on this:

maybe installing/ uninstalling single packages can be considered somehow a less frequent need, so one could argue whether the single dependencies are needed at all. Of course, this might be also "personal".

For one thing, I cannot find a useful example right now to have install/uninstall bound to some VS build events. By comparison with NuGet, it sounds like performing a NuGet package install/uninstall in relation to some build event, and I cannot see usefulness of this ... I'm guess I'm missing something from the big picture.

Install/uninstall are more related to project setup, than build events, so are somehow more "stable". Comparatively, it might be more useful to provide some kind of project-wide update / upgrade, e.g. similar to npm update, but then again, there should be a reasonable need for this.

[scottaddie]

maybe installing/ uninstalling single packages can be considered somehow a less frequent need, so one could argue whether the single dependencies are needed at all. Of course, this might be also "personal".

I agree that uninstalling individual packages is a less frequent need; however, it's useful for members of a team who aren't familiar with the inner workings of npm but want to uninstall a particular dependency which is no longer needed. This would save them from dropping out to the command shell and potentially messing something up.

For one thing, I cannot find a useful example right now to have install/uninstall bound to some VS build events. By comparison with NuGet, it sounds like performing a NuGet package install/uninstall in relation to some build event, and I cannot see usefulness of this ... I'm guess I'm missing something from the big picture.

Install/uninstall are more related to project setup, than build events, so are somehow more "stable". Comparatively, it might be more useful to provide some kind of project-wide update / upgrade, e.g. similar to npm update, but then again, there should be a reasonable need for this.

As for the event binding of install and uninstall, I tend to agree with you that it's probably not needed. I chose that as a quick example for testing purposes, without putting much thought into it.

[BrainCrumbz]

This would save them from dropping out to the command shell and potentially messing something up.

I tend to agree on this as well. I don't know if npm TRX can disable binding for some particular nodes: in that case, I'd say better to disable binding from whole install/uninstall subtrees.

[scottaddie]

@madskristensen I have another idea related to this issue. Per the docs, a publish will fail if the package.json file's private property is set to true. Here's an example of where that property is being set in the ASP.NET 5 / Core StarterWeb project template: https://github.com/aspnet/Templates/blob/dev/src/BaseTemplates/StarterWeb/package.json#L4.

Is there any interest in disabling or removing the publish node when the private property is set to true? Leaving it in place in this scenario would always result in a failure. Why display an option to the user that is guaranteed to result in failure?

[madskristensen]

@scottaddie Publish is one of those things that I doubt anyone will ever invoke from TRX. But if they do, don't you think they want to see the error message stating that it can't publish a package marked "private"?

[BrainCrumbz]

You could release extension as modified recently, and see as it goes, when you get further feedbacks from users.

[madskristensen]

@BrainCrumbz just did

[scottaddie]

@madskristensen Tough to say. I would invoke it from the command shell, but of course there are many other user personas out there. I am very interested to see the feedback from the latest enhancements.

[madskristensen]

I'm closing this issue since it was added to the latest release