-
Notifications
You must be signed in to change notification settings - Fork 592
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
[heft] Require a logging name for every operation #4469
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,7 @@ export interface IOperationExecutionOptions { | |
parallelism: number; | ||
terminal: ITerminal; | ||
|
||
requestRun?: (requestor?: string) => void; | ||
requestRun?: (requester: string) => void; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The requester could ultimately be multiple layers of identifiers; I guess we can just concatenate strings, but it might be better to make this an array of hierarchy nodes (e.g. Operation, file). When Rush receives one of these messages over IPC it'll get wrapped in a further node for the Rush-level Operation. |
||
} | ||
|
||
/** | ||
|
@@ -76,8 +76,8 @@ export class OperationExecutionManager { | |
for (const dependency of consumer.dependencies) { | ||
if (!operations.has(dependency)) { | ||
throw new Error( | ||
`Operation ${JSON.stringify(consumer.name)} declares a dependency on operation ` + | ||
`${JSON.stringify(dependency.name)} that is not in the set of operations to execute.` | ||
`Operation ${JSON.stringify(consumer.operationName)} declares a dependency on operation ` + | ||
`${JSON.stringify(dependency.operationName)} that is not in the set of operations to execute.` | ||
); | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,7 +31,7 @@ export interface IWatchLoopOptions { | |
/** | ||
* Logging callback when a run is requested (and hasn't already been). | ||
*/ | ||
onRequestRun: (requestor?: string) => void; | ||
onRequestRun: (requester: string) => void; | ||
/** | ||
* Logging callback when a run is aborted. | ||
*/ | ||
|
@@ -45,7 +45,7 @@ export interface IWatchLoopOptions { | |
*/ | ||
export interface IWatchLoopState { | ||
get abortSignal(): AbortSignal; | ||
requestRun: (requestor?: string) => void; | ||
requestRun: (requester: string) => void; | ||
} | ||
|
||
/** | ||
|
@@ -61,7 +61,7 @@ export class WatchLoop implements IWatchLoopState { | |
private _isRunning: boolean; | ||
private _runRequested: boolean; | ||
private _requestRunPromise: Promise<string | undefined>; | ||
private _resolveRequestRun!: (requestor?: string) => void; | ||
private _resolveRequestRun!: (requester: string) => void; | ||
|
||
public constructor(options: IWatchLoopOptions) { | ||
this._options = options; | ||
|
@@ -148,7 +148,7 @@ export class WatchLoop implements IWatchLoopState { | |
let runRequestedFromHost: boolean = true; | ||
let status: OperationStatus = OperationStatus.Ready; | ||
|
||
function requestRunFromHost(requestor?: string): void { | ||
function requestRunFromHost(requester: string): void { | ||
if (runRequestedFromHost) { | ||
return; | ||
} | ||
|
@@ -157,7 +157,7 @@ export class WatchLoop implements IWatchLoopState { | |
|
||
const requestRunMessage: IRequestRunEventMessage = { | ||
event: 'requestRun', | ||
requestor | ||
requester | ||
}; | ||
|
||
host.send!(requestRunMessage); | ||
|
@@ -193,7 +193,9 @@ export class WatchLoop implements IWatchLoopState { | |
|
||
try { | ||
status = await this.runUntilStableAsync(abortController.signal); | ||
this._requestRunPromise.finally(requestRunFromHost); | ||
this._requestRunPromise.finally(() => { | ||
requestRunFromHost('runIPCAsync'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dmichon-msft what is the appropriate name to supply here? In the current implementation it was implicitly getting mapped to (If we're going to be referring to requesters/operations by names, then we should provide a meaningful name in every code path, rather than printing awkward messages like "This job was started by an unknown source.") There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If |
||
}); | ||
} catch (err) { | ||
status = OperationStatus.Failure; | ||
return reject(err); | ||
|
@@ -224,16 +226,16 @@ export class WatchLoop implements IWatchLoopState { | |
/** | ||
* Requests that a new run occur. | ||
*/ | ||
public requestRun: (requestor?: string) => void = (requestor?: string) => { | ||
public requestRun: (requester: string) => void = (requester: string) => { | ||
if (!this._runRequested) { | ||
this._options.onRequestRun(requestor); | ||
this._options.onRequestRun(requester); | ||
this._runRequested = true; | ||
if (this._isRunning) { | ||
this._options.onAbort(); | ||
this._abortCurrent(); | ||
} | ||
} | ||
this._resolveRequestRun(requestor); | ||
this._resolveRequestRun(requester); | ||
}; | ||
|
||
/** | ||
|
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.
(Improved this error message)