[heft] Require a logging name for every operation (followup)#4953
[heft] Require a logging name for every operation (followup)#4953bartvandenende-wm wants to merge 4 commits intomicrosoft:mainfrom
Conversation
…ration names percolate through the call graphs - Fix spelling of "requestor" and make it non-optional
bartvandenende-wm
requested review from
D4N14L,
apostolisms,
dmichon-msft,
iclanton and
octogonz
as code owners
common/changes/@microsoft/rush/octogonz-heft-issue-4467_2024-10-01-08-08.json
Outdated
Show resolved
Hide resolved
common/changes/@microsoft/rush/octogonz-heft-issue-4467_2024-10-01-08-08.json
Outdated
Show resolved
Hide resolved
| readonly groupName: string | undefined; | ||
| lastState: IOperationState | undefined; | ||
| readonly name: string | undefined; | ||
| readonly operationName: string; |
There was a problem hiding this comment.
This is also a breaking API change. Is this rename necessary?
There was a problem hiding this comment.
I feel this change is beneficial to ensure we are more explicit in the naming. The purpose of name is easily lost outside of the operation-graph lib context. Specifically as downstream libs use this interface to implement custom operations like the heft TaskOperationRunner.
| // the reject callback in the promise is discarded so we ignore errors | ||
| .catch(() => {}) |
There was a problem hiding this comment.
Is this behavior correct/expected?
There was a problem hiding this comment.
the watchloop is using a deferred promise pattern, but only binds the resolve function per:
rushstack/libraries/operation-graph/src/WatchLoop.ts
Lines 264 to 266 in ca6e96b
There was a problem hiding this comment.
There was a problem hiding this comment.
The logic here is wrong. requestRunPromise resolves with the requestor, which is why it had directly passed requestRunFromHost as the callback.
| // the reject callback in the promise is discarded so we ignore errors | ||
| .catch(() => {}) |
There was a problem hiding this comment.
The logic here is wrong. requestRunPromise resolves with the requestor, which is why it had directly passed requestRunFromHost as the callback.
| @@ -60,7 +60,7 @@ export class WatchLoop implements IWatchLoopState { | |||
| private _isRunning: boolean; | |||
| private _runRequested: boolean; | |||
| private _requestRunPromise: Promise<string | undefined>; | |||
There was a problem hiding this comment.
| private _requestRunPromise: Promise<string | undefined>; | |
| private _requestRunPromise: Promise<string>; |
|
fixed by #5250 |
4 participants
Summary
Fixes #4467 and builds upon #4469 previously actioned by @octogonz
Details
When using the heft watch mode using
heft starta re-run triggered by a heft-plugin is not properly logged as requestor in the console. This can block teams from properly debugging of heft plugin related build issues.This PR forces an operation to always have a name so we can log it accordingly.
How it was tested
Redo repro steps from #4467
Impacted documentation
N/A