-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix(core): handle run-commands targets with no commands #31364
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.
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
View your CI Pipeline Execution ↗ for commit 622b68c.
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes an issue where Nx hangs when a run-commands target has no commands by simulating a successful process exit.
- Modify NoopChildProcess to simulate immediate task completion
- Update runCommands to return NoopChildProcess for empty commands arrays
- Add a test case to validate that an empty commands array returns a successful result
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
packages/nx/src/tasks-runner/running-tasks/noop-child-process.ts | Introduces exitCallbacks and outputCallbacks; simulates immediate exit in the constructor |
packages/nx/src/executors/run-commands/run-commands.impl.ts | Returns NoopChildProcess when commands array is empty |
packages/nx/src/executors/run-commands/run-commands.impl.spec.ts | Adds test for handling empty commands arrays |
for (const cb of this.exitCallbacks) { | ||
cb(this.results.code, this.results.terminalOutput); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'exitCallbacks' and 'outputCallbacks' arrays are defined and iterated over in the constructor but are never updated when callbacks are registered via onExit or onOutput. Consider either removing these arrays or updating the implementation to store and later invoke registered callbacks consistently.
for (const cb of this.exitCallbacks) { | |
cb(this.results.code, this.results.terminalOutput); | |
} | |
setTimeout(() => { | |
for (const cb of this.exitCallbacks) { | |
cb(this.results.code, this.results.terminalOutput); | |
} | |
}, 0); |
Copilot uses AI. Check for mistakes.
}, | ||
context | ||
); | ||
expect(result).toEqual(expect.objectContaining({ success: true })); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test expects a 'success' property on the result, but NoopChildProcess does not explicitly provide this. Consider adding a getter for 'success' (e.g., returning true when results.code equals 0) to align the implementation with the test expectations.
Copilot uses AI. Check for mistakes.
constructor(private results: { code: number; terminalOutput: string }) { | ||
// Call exit callbacks asynchronously to simulate task completion | ||
for (const cb of this.exitCallbacks) { | ||
cb(this.results.code, this.results.terminalOutput); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constructor is attempting to iterate through exitCallbacks
which is initialized as an empty array, so no callbacks will be executed at this point. Since callbacks are registered through the onExit
method after construction, consider modifying onExit
to either:
- Execute the callback immediately if called after construction:
onExit(cb: (code: number, terminalOutput: string) => void): void {
this.exitCallbacks.push(cb);
// Already completed, call immediately
cb(this.results.code, this.results.terminalOutput);
}
- Or use
setTimeout
to ensure callbacks are executed asynchronously:
constructor(private results: { code: number; terminalOutput: string }) {
setTimeout(() => {
for (const cb of this.exitCallbacks) {
cb(this.results.code, this.results.terminalOutput);
}
}, 0);
}
This will ensure proper simulation of an asynchronous process completion.
constructor(private results: { code: number; terminalOutput: string }) { | |
// Call exit callbacks asynchronously to simulate task completion | |
for (const cb of this.exitCallbacks) { | |
cb(this.results.code, this.results.terminalOutput); | |
} | |
} | |
constructor(private results: { code: number; terminalOutput: string }) { | |
// Defer callback execution to next event loop tick to simulate async task completion | |
setTimeout(() => { | |
for (const cb of this.exitCallbacks) { | |
cb(this.results.code, this.results.terminalOutput); | |
} | |
}, 0); | |
} |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
80bc137
to
ce49dc6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR ensures that run-commands
targets with an empty commands
array return immediately instead of hanging.
- Adds a
NoopChildProcess
implementation that emits a zero exit code and empty output. - Introduces an early return in
runCommands
when there are no commands. - Adds a unit test to verify the empty-commands behavior.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
packages/nx/src/tasks-runner/running-tasks/noop-child-process.ts | Expanded NoopChildProcess with onExit /onOutput APIs |
packages/nx/src/executors/run-commands/run-commands.impl.ts | Added early-return using NoopChildProcess for empty lists |
packages/nx/src/executors/run-commands/run-commands.impl.spec.ts | Added test case for handling an empty commands array |
Comments suppressed due to low confidence (1)
packages/nx/src/tasks-runner/running-tasks/noop-child-process.ts:18
- The
NoopChildProcess
class doesn’t exposesuccess
orterminalOutput
properties, but the new test asserts them directly. Add getters to forwardthis.results.code === 0
assuccess
andthis.results.terminalOutput
asterminalOutput
.
}
ce49dc6
to
622b68c
Compare
Current Behavior
Nx hangs when here is a
run-commands
target with no commands.Expected Behavior
Nx does not hang when there is a
run-commands
target with no commands.Related Issue(s)
Fixes #31345