Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[rush] Add hooks for pre and post rushx events #4286

Merged
merged 32 commits into from
Oct 6, 2023
Merged

[rush] Add hooks for pre and post rushx events #4286

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
bbba972
adding pre and post rushx events
nebosite Aug 18, 2023
bfff506
Adding Pro and Post event hooks to rushx
nebosite Aug 23, 2023
6d29a0e
Move exitcode handling out of _lauchRushXInternal
nebosite Sep 7, 2023
406fb69
Whoops accidentally checked in test code
nebosite Sep 7, 2023
d936b16
Special handling for process errors
nebosite Sep 7, 2023
8050af6
Quote unknown arguments
nebosite Sep 7, 2023
454e113
Rename INSIDERUSH to _RUSH_SUPPRESS_HOOKS
nebosite Sep 7, 2023
9517be9
Add debug parameter to rushx so it can be passed to hooks
nebosite Sep 7, 2023
39d1011
Don't print hook suppressed messages on recursive calls
nebosite Sep 8, 2023
f8a0be2
Add --ignorehooks and fix bug in hook runner
nebosite Sep 8, 2023
657eb84
Don't try to run hooks on --help
nebosite Sep 8, 2023
3ba9901
responding to code reviews
nebosite Oct 3, 2023
738b55f
Rush change
nebosite Oct 3, 2023
327fbe4
Merge remote-tracking branch 'remotes/origin/main' into neboste/rushx…
octogonz Oct 4, 2023
3815b1a
Fix some build warnings
octogonz Oct 4, 2023
c89d1dc
Fix a test failure (but I have some questions about this variable, se…
octogonz Oct 4, 2023
a93c500
Add more detail to change file
octogonz Oct 4, 2023
3d406c6
Document event hooks in rush.json template
octogonz Oct 4, 2023
ff5d1b9
Rename RUSH_SUPPRESS_HOOKS to _RUSH_SUPPRESS_RUSHX_HOOKS (because it …
octogonz Oct 4, 2023
914ab21
Rename EnvironmentVariableNames.RUSH_LIB_PATH to _RUSH_LIB_PATH to ma…
octogonz Oct 5, 2023
aaee1f2
Remove "-i" parameter per PR discussion
octogonz Oct 5, 2023
3065543
Refactor arguments to _launchRushXInternal
nebosite Oct 5, 2023
b7f94fd
Refactor environment variable name for hook suppression
nebosite Oct 5, 2023
ee01b51
Handle rushx hook errors as benign
nebosite Oct 5, 2023
710af8a
rolling back accidental change to rush and rushx scripts
nebosite Oct 5, 2023
2bd1390
Add environment var _RUSH_ORIGINAL_ARGS
nebosite Oct 5, 2023
5618ca8
Rename _RUSH_ORIGINAL_ARGS to a public environment variable RUSH_INVO…
octogonz Oct 5, 2023
6da0cf6
RUSH_INVOKED_ARGS should only be assigned when invoking hook scripts …
octogonz Oct 6, 2023
125f25c
putting back alreadyReportedNodeTooNewError
nebosite Oct 6, 2023
04c723d
rolling back accidental change to rush and rushx scripts
nebosite Oct 6, 2023
9236fac
Merge branch 'neboste/rushx_hooks' of github.com:nebosite/rushstack i…
nebosite Oct 6, 2023
00d2c19
make _launchRushXInternal private
nebosite Oct 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion common/reviews/api/rush-lib.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,8 +202,10 @@ export const EnvironmentVariableNames: {
export enum Event {
postRushBuild = 4,
postRushInstall = 2,
postRushx = 6,
preRushBuild = 3,
preRushInstall = 1
preRushInstall = 1,
preRushx = 5
}

// @beta
Expand Down
10 changes: 9 additions & 1 deletion libraries/rush-lib/src/api/EventHooks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,15 @@ export enum Event {
/**
* Post Rush build event
*/
postRushBuild = 4
postRushBuild = 4,
/**
* Start of rushx execution event
*/
preRushx = 5,
/**
* End of rushx execution event
*/
postRushx = 6
}

/**
Expand Down
2 changes: 1 addition & 1 deletion libraries/rush-lib/src/api/Rush.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ export class Rush {
options = Rush._normalizeLaunchOptions(options);

Rush._assignRushInvokedFolder();
RushXCommandLine._launchRushXInternal(launcherVersion, { ...options });
RushXCommandLine.launchRushX(launcherVersion, options);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@octogonz octogonz Oct 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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. 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

/**
Expand Down
193 changes: 103 additions & 90 deletions libraries/rush-lib/src/cli/RushXCommandLine.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ import { Rush } from '../api/Rush';
import { RushConfiguration } from '../api/RushConfiguration';
import { NodeJsCompatibility } from '../logic/NodeJsCompatibility';
import { RushStartupBanner } from './RushStartupBanner';
import { EventHooksManager } from '../logic/EventHooksManager';
import { Event } from '../api/EventHooks';

/**
* @internal
Expand Down Expand Up @@ -45,129 +47,139 @@ interface IRushXCommandLineArguments {
}

export class RushXCommandLine {
public static launchRushX(launcherVersion: string, isManaged: boolean): void {
RushXCommandLine._launchRushXInternal(launcherVersion, { isManaged });
public static launchRushX(launcherVersion: string, options: ILaunchRushXInternalOptions): void {
try {
const args: IRushXCommandLineArguments = this._getCommandLineArguments();
nebosite marked this conversation as resolved.
Show resolved Hide resolved
const rushConfiguration: RushConfiguration | undefined = RushConfiguration.tryLoadFromDefaultLocation({
showVerbose: false
});
const eventHooksManager: EventHooksManager | undefined = rushConfiguration
? new EventHooksManager(rushConfiguration)
: undefined;

const ignoreHooks = process.env.INSIDERUSH === '1';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Collaborator

Choose a reason for hiding this comment

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

@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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@octogonz octogonz Oct 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. Good name suggestion- I changed it to _RUSH_RECURSIVE_RUSHX_CALL

eventHooksManager?.handle(Event.preRushx, false /* isDebug */, ignoreHooks);
nebosite marked this conversation as resolved.
Show resolved Hide resolved
RushXCommandLine._launchRushXInternal(launcherVersion, options, rushConfiguration, args);
eventHooksManager?.handle(Event.postRushx, false /* isDebug */, ignoreHooks);
nebosite marked this conversation as resolved.
Show resolved Hide resolved
} catch (error) {
console.log(colors.red('Error: ' + (error as Error).message));
}
}

/**
* @internal
*/
public static _launchRushXInternal(launcherVersion: string, options: ILaunchRushXInternalOptions): void {
public static _launchRushXInternal(
launcherVersion: string,
options: ILaunchRushXInternalOptions,
rushConfiguration: RushConfiguration | undefined,
args: IRushXCommandLineArguments
nebosite marked this conversation as resolved.
Show resolved Hide resolved
): void {
// Node.js can sometimes accidentally terminate with a zero exit code (e.g. for an uncaught
// promise exception), so we start with the assumption that the exit code is 1
// and set it to 0 only on success.
process.exitCode = 1;

const args: IRushXCommandLineArguments = this._getCommandLineArguments();

if (!args.quiet) {
RushStartupBanner.logStreamlinedBanner(Rush.version, options.isManaged);
}
// Are we in a Rush repo?
NodeJsCompatibility.warnAboutCompatibilityIssues({
isRushLib: true,
alreadyReportedNodeTooNewError: !!options.alreadyReportedNodeTooNewError,
rushConfiguration
});

// Find the governing package.json for this folder:
const packageJsonLookup: PackageJsonLookup = new PackageJsonLookup();

const packageJsonFilePath: string | undefined = packageJsonLookup.tryGetPackageJsonFilePathFor(
process.cwd()
);
if (!packageJsonFilePath) {
console.log(colors.red('This command should be used inside a project folder.'));
console.log(
`Unable to find a package.json file in the current working directory or any of its parents.`
);
return;
}

try {
// Are we in a Rush repo?
const rushConfiguration: RushConfiguration | undefined = RushConfiguration.tryLoadFromDefaultLocation({
showVerbose: false
});
NodeJsCompatibility.warnAboutCompatibilityIssues({
isRushLib: true,
alreadyReportedNodeTooNewError: !!options.alreadyReportedNodeTooNewError,
rushConfiguration
});

// Find the governing package.json for this folder:
const packageJsonLookup: PackageJsonLookup = new PackageJsonLookup();

const packageJsonFilePath: string | undefined = packageJsonLookup.tryGetPackageJsonFilePathFor(
process.cwd()
if (rushConfiguration && !rushConfiguration.tryGetProjectForPath(process.cwd())) {
nebosite marked this conversation as resolved.
Show resolved Hide resolved
// GitHub #2713: Users reported confusion resulting from a situation where "rush install"
// did not install the project's dependencies, because the project was not registered.
console.log(
colors.yellow(
'Warning: You are invoking "rushx" inside a Rush repository, but this project is not registered in rush.json.'
)
);
if (!packageJsonFilePath) {
console.log(colors.red('This command should be used inside a project folder.'));
console.log(
`Unable to find a package.json file in the current working directory or any of its parents.`
);
return;
}
}

if (rushConfiguration && !rushConfiguration.tryGetProjectForPath(process.cwd())) {
// GitHub #2713: Users reported confusion resulting from a situation where "rush install"
// did not install the project's dependencies, because the project was not registered.
console.log(
colors.yellow(
'Warning: You are invoking "rushx" inside a Rush repository, but this project is not registered in rush.json.'
)
);
}
const packageJson: IPackageJson = packageJsonLookup.loadPackageJson(packageJsonFilePath);

const packageJson: IPackageJson = packageJsonLookup.loadPackageJson(packageJsonFilePath);
const projectCommandSet: ProjectCommandSet = new ProjectCommandSet(packageJson);

const projectCommandSet: ProjectCommandSet = new ProjectCommandSet(packageJson);
if (args.help) {
RushXCommandLine._showUsage(packageJson, projectCommandSet);
return;
}

if (args.help) {
RushXCommandLine._showUsage(packageJson, projectCommandSet);
return;
}
const scriptBody: string | undefined = projectCommandSet.tryGetScriptBody(args.commandName);

const scriptBody: string | undefined = projectCommandSet.tryGetScriptBody(args.commandName);
if (scriptBody === undefined) {
console.log(
colors.red(
`Error: The command "${args.commandName}" is not defined in the` +
` package.json file for this project.`
)
);

if (scriptBody === undefined) {
if (projectCommandSet.commandNames.length > 0) {
console.log(
colors.red(
`Error: The command "${args.commandName}" is not defined in the` +
` package.json file for this project.`
)
'\nAvailable commands for this project are: ' +
projectCommandSet.commandNames.map((x) => `"${x}"`).join(', ')
);

if (projectCommandSet.commandNames.length > 0) {
console.log(
'\nAvailable commands for this project are: ' +
projectCommandSet.commandNames.map((x) => `"${x}"`).join(', ')
);
}

console.log(`Use ${colors.yellow('"rushx --help"')} for more information.`);
return;
}

let commandWithArgs: string = scriptBody;
let commandWithArgsForDisplay: string = scriptBody;
if (args.commandArgs.length > 0) {
// This approach is based on what NPM 7 now does:
// https://github.com/npm/run-script/blob/47a4d539fb07220e7215cc0e482683b76407ef9b/lib/run-script-pkg.js#L34
const escapedRemainingArgs: string[] = args.commandArgs.map((x) => Utilities.escapeShellParameter(x));
console.log(`Use ${colors.yellow('"rushx --help"')} for more information.`);
return;
}

commandWithArgs += ' ' + escapedRemainingArgs.join(' ');
let commandWithArgs: string = scriptBody;
let commandWithArgsForDisplay: string = scriptBody;
if (args.commandArgs.length > 0) {
// This approach is based on what NPM 7 now does:
// https://github.com/npm/run-script/blob/47a4d539fb07220e7215cc0e482683b76407ef9b/lib/run-script-pkg.js#L34
const escapedRemainingArgs: string[] = args.commandArgs.map((x) => Utilities.escapeShellParameter(x));

// Display it nicely without the extra quotes
commandWithArgsForDisplay += ' ' + args.commandArgs.join(' ');
}
commandWithArgs += ' ' + escapedRemainingArgs.join(' ');

if (!args.quiet) {
console.log(`> ${JSON.stringify(commandWithArgsForDisplay)}\n`);
}
// Display it nicely without the extra quotes
commandWithArgsForDisplay += ' ' + args.commandArgs.join(' ');
}

const packageFolder: string = path.dirname(packageJsonFilePath);

const exitCode: number = Utilities.executeLifecycleCommand(commandWithArgs, {
rushConfiguration,
workingDirectory: packageFolder,
// If there is a rush.json then use its .npmrc from the temp folder.
// Otherwise look for npmrc in the project folder.
initCwd: rushConfiguration ? rushConfiguration.commonTempFolder : packageFolder,
handleOutput: false,
environmentPathOptions: {
includeProjectBin: true
}
});
if (!args.quiet) {
console.log(`> ${JSON.stringify(commandWithArgsForDisplay)}\n`);
}

if (exitCode > 0) {
console.log(colors.red(`The script failed with exit code ${exitCode}`));
const packageFolder: string = path.dirname(packageJsonFilePath);

const exitCode: number = Utilities.executeLifecycleCommand(commandWithArgs, {
rushConfiguration,
workingDirectory: packageFolder,
// If there is a rush.json then use its .npmrc from the temp folder.
// Otherwise look for npmrc in the project folder.
initCwd: rushConfiguration ? rushConfiguration.commonTempFolder : packageFolder,
handleOutput: false,
environmentPathOptions: {
includeProjectBin: true
}
});

process.exitCode = exitCode;
} catch (error) {
console.log(colors.red('Error: ' + (error as Error).message));
if (exitCode > 0) {
console.log(colors.red(`The script failed with exit code ${exitCode}`));
}
octogonz marked this conversation as resolved.
Show resolved Hide resolved

process.exitCode = exitCode;
}

private static _getCommandLineArguments(): IRushXCommandLineArguments {
Expand Down Expand Up @@ -206,6 +218,7 @@ export class RushXCommandLine {
if (unknownArgs.length > 0) {
// Future TODO: Instead of just displaying usage info, we could display a
// specific error about the unknown flag the user tried to pass to rushx.
console.log(colors.red(`Unknown arguments: ${unknownArgs.join(', ')}`));
nebosite marked this conversation as resolved.
Show resolved Hide resolved
help = true;
}

Expand Down
8 changes: 4 additions & 4 deletions libraries/rush-lib/src/cli/test/RushXCommandLine.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ describe(RushXCommandLine.name, () => {
process.argv = ['node', 'startx.js', '--help'];
executeLifecycleCommandMock!.mockReturnValue(0);

RushXCommandLine.launchRushX('', true);
RushXCommandLine.launchRushX('', { isManaged: true });

expect(executeLifecycleCommandMock).not.toHaveBeenCalled();
expect(logMock!.mock.calls).toMatchSnapshot();
Expand All @@ -106,7 +106,7 @@ describe(RushXCommandLine.name, () => {
process.argv = ['node', 'startx.js', 'build'];
executeLifecycleCommandMock!.mockReturnValue(0);

RushXCommandLine.launchRushX('', true);
RushXCommandLine.launchRushX('', { isManaged: true });

expect(executeLifecycleCommandMock).toHaveBeenCalledWith('an acme project build command', {
rushConfiguration,
Expand All @@ -124,7 +124,7 @@ describe(RushXCommandLine.name, () => {
process.argv = ['node', 'startx.js', '--quiet', 'build'];
executeLifecycleCommandMock!.mockReturnValue(0);

RushXCommandLine.launchRushX('', true);
RushXCommandLine.launchRushX('', { isManaged: true });

expect(executeLifecycleCommandMock).toHaveBeenCalledWith('an acme project build command', {
rushConfiguration,
Expand All @@ -142,7 +142,7 @@ describe(RushXCommandLine.name, () => {
process.argv = ['node', 'startx.js', 'asdf'];
executeLifecycleCommandMock!.mockReturnValue(0);

RushXCommandLine.launchRushX('', true);
RushXCommandLine.launchRushX('', { isManaged: true });

expect(executeLifecycleCommandMock).not.toHaveBeenCalled();
expect(logMock!.mock.calls).toMatchSnapshot();
Expand Down
2 changes: 1 addition & 1 deletion libraries/rush-lib/src/logic/EventHooksManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ export class EventHooksManager {
console.error(
nebosite marked this conversation as resolved.
Show resolved Hide resolved
'\n' +
colors.yellow(
`Event hook "${script}" failed. Run "rush" with --debug` +
`Event hook "${script}" failed: ${error}\nRun "rush" with --debug` +
nebosite marked this conversation as resolved.
Show resolved Hide resolved
` to see detailed error information.`
)
);
Expand Down
14 changes: 14 additions & 0 deletions libraries/rush-lib/src/schemas/rush.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,20 @@
"items": {
"type": "string"
}
},
"preRushx": {
"description": "The list of scripts to run before rushx starts.",
"type": "array",
"items": {
"type": "string"
}
},
"postRushx": {
"description": "The list of scripts to run after rushx finishes.",
"type": "array",
"items": {
"type": "string"
}
}
},
"additionalProperties": false
Expand Down
3 changes: 3 additions & 0 deletions libraries/rush-lib/src/utilities/Utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -504,6 +504,9 @@ export class Utilities {
}
});

// Communicate to downstream calls that they are running inside a top-level rush command
environment.INSIDERUSH = '1';

return spawnFunction(shellCommand, [commandFlags, command], {
cwd: options.workingDirectory,
shell: useShell,
Expand Down
Loading