Some small cleanup changes pulled out of the quiet logger PR #838
Conversation
Adds some more events, logs them more consistently, puts the logger in control of how it's wrapped when in watch mode.
| @@ -2,6 +2,6 @@ | |||
| "javascript.preferences.importModuleSpecifierEnding": "js", | |||
| "typescript.preferences.importModuleSpecifierEnding": "js", | |||
| "editor.defaultFormatter": "esbenp.prettier-vscode", | |||
| "typescript.tsdk": "node_modules\\typescript\\lib", | |||
| "typescript.tsdk": "node_modules/typescript/lib", | |||
There was a problem hiding this comment.
Could this impact Windows users?
There was a problem hiding this comment.
Often / works everywhere but \ only works on Windows. If it does then we could likely remove this by then
src/analyzer.ts
Outdated
| let rootScriptConfig = undefined; | ||
| if (analyzeResult.config.ok) { | ||
| rootScriptConfig = analyzeResult.config.value; | ||
| } |
There was a problem hiding this comment.
This is very nice. What do you think about inlining this as a ternary in value position of rootScriptConfig? Then you can skip this if there isn't a logger.
Although it may be a bit less readable. Up to you.
src/event.ts
Outdated
| /** | ||
| * The analysis phase for an execution has begun, where we load package.json | ||
| * files, analyze their wireit configs, and build the dependency graph. | ||
| */ |
There was a problem hiding this comment.
Update to reflect completion of analysis.
| this._logger.log({ | ||
| script: this._config, | ||
| type: 'info', | ||
| detail: 'service-process-started', |
There was a problem hiding this comment.
Nit: Is it worth reflecting the state names in detail? E.g., service-process-starting?
There was a problem hiding this comment.
Starting makes sense to the internal state machine of ServiceScriptExecution, but for everywhere else at this point we have a child process now, so the process has started.
| @@ -262,6 +262,15 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand<ServiceScri | |||
| entireExecutionAborted, | |||
| adoptee, | |||
| }; | |||
| // Doing this here ensures that we always log when the | |||
| // service stops, no matter how that happens. | |||
There was a problem hiding this comment.
Are there other conditions outside the stopping and failing states that could be missed otherwise?
I do like hanging this off the promise.
edit: is child.kill({force: true}); one of these additional cases that are now handled?
There was a problem hiding this comment.
Before this change we'd only log that it stopped if we stopped it outselves and it was in certain specific states. This way, no matter how it dies (e.g. it chooses to call process.exit()), we inform the logger about it.
| @@ -31,7 +31,7 @@ interface EventBase<T extends PackageReference = ScriptReference> { | |||
| // Success events | |||
| // ------------------------------- | |||
|
|
|||
| type Success = ExitZero | NoCommand | Fresh | Cached; | |||
| export type Success = ExitZero | NoCommand | Fresh | Cached; | |||
There was a problem hiding this comment.
Nit, when exporting should add a jsdoc comment.
src/test/util/test-rig.ts
Outdated
| @@ -318,7 +322,11 @@ class ExecResult { | |||
| // own process group. Passing the negative of the child's pid kills all | |||
| // processes in the group (without the negative only the leader "sh" | |||
| // process would be killed). | |||
| process.kill(-this._child.pid, 'SIGINT'); | |||
| if (force) { | |||
| process.kill(-this._child.pid, 9); | |||
There was a problem hiding this comment.
Nit: thoughts about using 'SIGKILL' instead of the number?
src/event.ts
Outdated
|
|
||
| /** | ||
| * A service started running and if it has a readyWhen condition, | ||
| * that condition is met. | ||
| */ | ||
| export interface ServiceStarted extends InfoBase { | ||
| detail: 'service-started'; |
There was a problem hiding this comment.
How about renaming this old event to service-ready, to distinguish it more clearly from service-process-started? The "ready" terminology is what we use in the configuration file and docs to mean that the ready condition, if there is one, has been satisfied (e.g. "Server started" was logged).
There was a problem hiding this comment.
+1, done. Also logged [serviceName] Service ready in the default logger instead of [serviceName] Service started
#836 is way too big, this pulls some of the incidental improvements out of it so they can be reviewed in isolation