madskristensen / NpmTaskRunner

Visual Studio extension
Other
88 stars 32 forks source link

[Feature request] Nest pre- and post- script commands #19

Closed BrainCrumbz closed 8 years ago

BrainCrumbz commented 8 years ago

Installed product versions

When npm finds a script command (e.g. bar) that also has a related pre- or post- command (e.g. prebar or postbar), it will run those in sequence. By taking advantage of such feature, the Task Runner Explorer list quickly becomes crowded with less-than-meaningful tasks:

trx crowded

It might make sense to reduce crowding by nesting related group of commands under the main one, so that they are usually not visible, but still accessible if one really wants to run those. So e.g. something like:

-+- build-dev
 |--- postbuild-dev

-+- serve-prod
 |--- preserve-prod

Don't know if Task list supports nesting at the command level, although graphically there already is between package.json and Scripts items, and then with its children.

madskristensen commented 8 years ago

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?

@scottaddie What do you think?

BrainCrumbz commented 8 years ago

If there's a postbar command, but no bar, then postbar is just another command, it has nothing special to npm. So, it seems to me that nesting only makes sense when the pre-/post- thing is working, not otherwise.

Thus, to your example, if there's a postinstall but no install, I'd leave postinstall as a first level node. This behaviour is good not only for somehow "standard" commands (like install), but for any command.

If task runner finds some predate command, it has no way to know if that is the pre- phase of some date command, or it's one of its own. The only way npm has to figure that out is, if the date command exists as well.

madskristensen commented 8 years ago

That makes sense to me.

madskristensen commented 8 years ago

It now organizes pre- and -post under their parent task:

image

Please download the CI build to verify the behavior.

BrainCrumbz commented 8 years ago

Wow, that was quick! :smile: Sure, will do.

BrainCrumbz commented 8 years ago

Looks perfect!

task runner explorer nested

... it pairs nicely with the Asp.Net core/ WebPack/ Angular2/ TypeScript starter project we're working at: https://github.com/BrainCrumbz/AWATTS
(yes, I know there's also a webpack task runner, but this suits us better)

Thank you!

madskristensen commented 8 years ago

That project looks really awesome. Thanks for sharing. Please let me know if you run into any issues with Visual Studio or any of the extensions you use so we can fix them.

I'll release a new version of this extension now.

BrainCrumbz commented 8 years ago

Please wait a sec for new release...

madskristensen commented 8 years ago

Wazzup?

BrainCrumbz commented 8 years ago

Cooking a related issue... (still writing it) #20

scottaddie commented 8 years ago

@madskristensen Thumbs up to the nesting idea! :+1:

I've also had some thoughts brewing in my head that possibly take this idea a step further. What do you think about visually separating the "predefined" npm scripts (e.g., install, test, start) from "custom" npm scripts created by the developer? This could be accomplished by associating each script type with a different parent node. Instead of a single "Scripts" node, we could have something like "Lifecycle" and "Custom". Maybe there's a better term than lifecycle.

These "predefined" script names can all be found within the scripts property here. They're also documented in the bulleted list here.

Consider the following example:

scripts

I would expect the following scripts to appear under the proposed "Lifecycle" node:

  1. build
  2. postinstall
  3. start
  4. test

The remaining scripts would appear under the proposed "Custom" node. Thoughts?

madskristensen commented 8 years ago

Isn't the issue that the predefined commands aren't really lifecycle events? For instance, test and restart aren't really lifecycle events but commands that the user has to execute manually

scottaddie commented 8 years ago

Agreed. "Lifecycle" probably isn't the right term to use for this node heading. "Predefined" may be a better term.

For those who are migrating to npm scripts from a task runner such as Gulp, there may be several scripts to define. This is where it may be useful to make a clear distinction between what's a reserved script name and what's not. The visual separation into 2 categories would be a hint to developers that there's something special about those predefined scripts (e.g., that one may be triggered by running something else).

BrainCrumbz commented 8 years ago

Not sure this is already the case, but I'd suggest to move this discussion off to #20 , if you agree as well. After all, that issue is spot on, about default scripts

scottaddie commented 8 years ago

Agreed. Conversation has moved to there.