[rush] Add hooks for pre and post rushx events

[nebosite]

Summary

Adding hooks similar to what we have in rush. The hooks are needed to help analyze local rushx calls.

Details

Changes:

• Added two new hooks to rush.json: preRushx and postRushx
• Added calls inside rushx to call the new hooks
• Refactored internal and external rushx launch calls to simplify adding of hooks
• Added an environment variable to communicate when rushx calls are downstream to prevent hook calls for recursive calls to rushx. (I expect there might already exist some mechanism for this.)
• Added a few output changes to give a little more information on errors

Performance impact:

• The rush configuration is already being loaded, so no change from that.
• In the case where I added some simple hooks (pre & post), the overhead was about 100 ms to call them both.

Backward compatability:

• There is one parameter change to the public API lauchRushX. As far as I can tell, this only broke tests, which I fixed.
• Code path is essentially the same except for the addition of the hook calls. These are called in a process, so side effects are not expected.
• The guard provided by the INSIDERUSH environment variable should prevent a runaway performance problem with hooks when rushx is called in a deep recursion.

Example output:

➜  common git:(ejorgens/HZ-29311) ✗ rushx build

Executing event hooks for preRushx

Event hooks finished. (0.19 seconds)
Rush Multi-Project Build Tool 5.102.0 (unmanaged) - Node.js 16.14.2 (LTS)
> "rushx clean && rushx ibuild"

Rush Multi-Project Build Tool 5.102.0 (unmanaged) - Node.js 16.14.2 (LTS)
> "rimraf coverage dist"

Rush Multi-Project Build Tool 5.102.0 (unmanaged) - Node.js 16.14.2 (LTS)
> "tsc"


Executing event hooks for postRushx

Event hooks finished. (0.20 seconds)

How it was tested

Ran local tests => passed
Ran rushx build on a small project with just prehook => worked
Ran with just posthook => worked
Ran with both prehook and posthook => worked
Ran with bad path in hook => reported error as expected
Ran with no hooks => worked
Additional manual tests
[x] Verify event hooks called only for top level rush call
[x] "Hooks not run" message should now show on recursive calls
[x] running with --ignorehooks should not call any hooks
[x] Running a non-existent script should report error with error code 1
[x] Running a script that returns an error should report the error and return the identical error code
[x] Running in a folder without a package.json should fail with correct message and error code 1
[x] Running --help should have error code 0, not run hooks, and should show expected usage output
[x] Running with --debug should show hook output

Impacted documentation

Need to update events here: https://api.rushstack.io/pages/rush-lib.event/

[nebosite]

See the changes in RushXCommandLine.ts for the technical reasons motivating this change. It also doesn't seem kosher to call the internal version from here.

[dmichon-msft]

It's marked internal, not private. The marker is that it is expected to be called from within the package and not externally. These are in the same package.

[nebosite]

I see. Shall I leave the code as-is, or add another layer to contain the code added to launchRushX?

[iclanton]

I'm trying to remember why we did this like this. @octogonz, do you recall?

[octogonz]

Looking at the code, the sort-of-public entry points are:

export class Rush {
    static launch(launcherVersion: string, arg: ILaunchOptions): void;
    static launchRushPnpm(launcherVersion: string, options: ILaunchOptions): void;
    static launchRushX(launcherVersion: string, options: ILaunchOptions): void;

They are not marked as @internal but instead have doc comments instructing third parties NOT to use them, because they are only meant to be accessed by the @microsoft/rush CLI front end.

Why didn't we label them as @internal? I think the answer is that Rush.launch() is ancient and predates API Extractor, and then the other two just continued the same pattern. We "should" fix that, except that doing so would break compatibility with all previous versions of the @microsoft/rush front end.

The situation of RushXCommandLine is more mysterious, since this class is not actually exported by index.d.ts at all, yet it has both an @internal and regular method:

export class RushXCommandLine {
  public static launchRushX(launcherVersion: string, isManaged: boolean): void {
    RushXCommandLine._launchRushXInternal(launcherVersion, { isManaged });
  }

  /**
   * @internal
   */
  public static _launchRushXInternal(launcherVersion: string, options: ILaunchRushXInternalOptions): void {

I forget the details, but my guess is that there is probably one early release of @microsoft/rush that did import RushXCommandLine.launchRushX() but then we quickly replaced that with Rush.launchRushX() to reduce the API surface. Rush 6 would be a good opportunity to clean up this contract.

[nebosite]

@octogonz To be clear, for this particular line of code, it is a repointing, not a renaming. Both launchRushx and _launchRushXInternal have the same names and this repointing is necessary to preserve the same functionality as before.

[octogonz]

That's true here, but in the RushXCommandLine class, this API:

  public static launchRushX(launcherVersion: string, isManaged: boolean): void {

...got renamed to this:

 public static launchRushX(launcherVersion: string, options: ILaunchRushXInternalOptions): void {

I'm not necessarily opposed to cleaning this up, but since it's old code and we didn't remember whether there are @microsoft/rush releases that access it, it might require some additional review.

Whereas the RushXCommandLine._launchRushXInternal() is explicitly marked as @internal, so it seems very safe to change that one however you like, including its ILaunchRushXInternalOptions interface.

[octogonz]

(Hold on, I'm going to just look at the Git history.)

[octogonz]

• The original implementation of rushx did not have RushXCommandLine at all:
  [rush] Add "rushx" binary for project-specific commands #678
• That file was added by some refactoring, but initially without introducing the @internal member _launchRushXInternal():
  [rush] Add support for "global" custom commands #684
• The @internal member was added in this later PR:
  [rush] Update rush NodeJS warning messages and add option to suppress non-LTS warning messages. #1388

The review comments don't explain why we added an @internal member to RushXCommandLine, but perhaps it was this same paranoia about whether RushXCommandLine.launchRushX() was a @microsoft/rush version selector contract or not.

So, looking at this code, I think we can conclude that RushXCommandLine is entirely internal to rush-lib, and therefore we should remove the misleading @internal markers from that class, and you're free to refactor it however you like. 🙂

[nebosite]

OK, I removed the internal markers and refactored the call to _launchRushXInternal to take a single options argument.

[nebosite]

This generates a little ugliness in the output, because we will see messages about hooks being ignored. The upside is that we don't have to change the event handler.

[octogonz]

Rush's environment variable names are always prefixed with RUSH_ (or _RUSH_ for internal variables). They must be documented in EnvironmentConfiguration.ts. Normally that file handles validating them, however for a performance sensitive operation such as rushx, it is okay to read directly from process.env.

The name INSIDERUSH could be more clear. Looking at the code, it seems that the intent is really to skip event hooks in child processes, because we are already processing a hook? If so, a better name might be _RUSH_INVOKING_HOOK or _RUSH_SUPPRESS_HOOKS. If this is an internal communication mechanism for a specific feature, it's better to make the name highly specific, rather than giving it a general purpose name (that temps other people to rely on it, and then they will get broken if we have to change something).

[nebosite]

Fixed

[octogonz]

@nebosite I have renamed this variable to _RUSH_SUPPRESS_RUSHX_HOOKS because:

1. It seems to only suppress rush.json eventHooks for rushx, not the other rush events
2. The logic (detecting nested invocations) seems potentially tricky, so making it internal will give us more flexibility to adjust the semantics in the future, and
3. End users can use --ignore-hooks already, so they probably don't have a strong need for an environment variable anyway. (If that need arises, we can expose a separate public variable that exactly corresponds to --ignore-hooks)

[nebosite]

OK.

I've been thinking about this some more and I'm not sure we want to use an environment variable this way. IMO, environment variables are better for communicating context than they are at communicating imperative. A SUPPRESS_HOOKS variable operates like an imperative, which obscures the reason that we are ignoring the hooks. It leads to code like this:

    if(!process.env["SUPPRESS_HOOKS"]) {
        callhooks();
    }

That code looks perfectly correct, but:

1. it isn't telling me anything about why the hooks are suppressed in the first place. Any bug is going to be in some distant code that sets the value of SUPPRESS_HOOKS.
2. This setup also makes it harder for a developer to keep the suppress hooks story correct. There might be multiple places in the code where a decision is made to suppress hooks or not. If the decision logic for when to suppress hooks changes, we have to search for all the places we might set it.
3. It is difficult to write and maintain a cohesive unit test to verify when hooks are called correctly

For the alternative, consider a context approach with a variable such as IS_RECURSIVE_CALL:

    if(!process.env["IS_RECURSIVE_CALL"]) {
        callhooks();
    }

Using context gives several improvements:

1. we can be more certain about the semantics of a conext variable. It would likely get set right before calling a new process where is is clear from the local context that we are in fact making a recursive call.
2. Changing the logic for when to set a context variable is less likely to change, and when it does change it is less likely to create a regression because I'm not making a decision about what downstream logic needs to execute.
3. Easier to read because I see the "why" right next to the hook call.
4. Easier to unit test. With one place to add new logic, it is trivial to unit test something like this:

    if(!process.env["IS_RECURSIVE_CALL"]
        && !arguments.ignoreHooks
        && !machineIsInEgypt) {
            callhooks();
    }

I'll leave this unresolved so that you read it. It's your call to leave the the current code in place, but I think a context variable would be better here.

[octogonz]

Agreed, the main point of my feedback was that this environment variable is an internal implementation detail and may be subject to change in the future. (For example, what if rushx invokes heft which runs a Jest test of Rush's own code that tries to test the hooks feature itself, but that test fails because the environment variable got inherited into that context. Or rushx gets invoked by the rush build event hook, which is a different nesting scenario that is NOT suppressed in the current implementation. When exactly is it best to suppress? We need to use the feature a bit before we truly understand all the edge cases.)

Adding the _ prefix to _RUSH_SUPPRESS_RUSHX_HOOKS communicates to Rush users that they should NOT be setting/reading this variable, because its meaning may change, or it may be removed entirely. That gives us some flexibility to refine it in the future.

You can call the variable whatever makes the most intuitive sense to you, but my suggestion is to make the name very narrow. (For example IS_RECURSIVE_CALL could be narrowed to _RUSH_RECURSIVE_RUSHX_CALL.) Doing so will discourage people from thinking they know what this variable does and disregarding the "internal" designation.

[nebosite]

OK. Good name suggestion- I changed it to _RUSH_RECURSIVE_RUSHX_CALL

[dmichon-msft]

It's marked internal, not private. The marker is that it is expected to be called from within the package and not externally. These are in the same package.

[octogonz]

@iclanton @dmichon-msft This PR has been under review for a long time. Let's try to get it merged and released.

[octogonz]

@nebosite This is not correct. The launchRushX() is the entry point for the rushx CLI binary (via the VersionSelector system). The options such as alreadyReportedNodeTooNewError are important features. We cannot simply discard them here.

[nebosite]

Ah, I see. I didn't think about the API. I'll fix it tongith.

[octogonz]

🚀 This feature was released with Rush 5.109.1