Conversation
| * Resolves when this child process ends. | ||
| */ | ||
| readonly completed: Promise< | ||
| get completed(): Promise< |
There was a problem hiding this comment.
can this just be:
readonly completed = this._completed.promise;
There was a problem hiding this comment.
If there's even going to be a new run of the child process with new Deferreds for started and completion, then the getters are safer.
There was a problem hiding this comment.
Good point, done!
If there's even going to be a new run of the child process with new Deferreds for started and completion, then the getters are safer.
The child process will only be re-used if it doesn't need to be restarted. Otherwise there will be a new ScriptChildProcess instance. So this is safe.
| * Resolves when this child process ends. | ||
| */ | ||
| readonly completed: Promise< | ||
| get completed(): Promise< |
There was a problem hiding this comment.
If there's even going to be a new run of the child process with new Deferreds for started and completion, then the getters are safer.
| break; | ||
| } | ||
| default: { | ||
| const never: never = this._state; |
There was a problem hiding this comment.
Can this be folded into the previous case?
There was a problem hiding this comment.
No, it's a different error, and it has a never assignment to prove to TypeScript that it can't happen. Or did I misunderstand somethning?
| } | ||
| default: { | ||
| const never: never = this._state; | ||
| const exception = new Error( |
There was a problem hiding this comment.
If you want the Error to have a stack trace, then you can throw it inside a try/catch and call reject() after it's caught.
There was a problem hiding this comment.
Cool trick. In this case the error string is unique, so I don't think a trace trace really adds much value.
| }); | ||
|
|
||
| this._child.on('close', (status, signal) => { | ||
| if (this._state === 'killing') { |
There was a problem hiding this comment.
Do you need to ensure or assert that _started is settled in any of these cases?
There was a problem hiding this comment.
I don't think so. close should only happen after spawn or error.
| dependencies: config.dependencies as Array<Dependency<ScriptConfig>>, | ||
| // Unfortunately TypeScript doesn't narrow the ...config spread, so we | ||
| // have to assign explicitly. | ||
| service: config.service, |
There was a problem hiding this comment.
Can you cast the config: ...(config as ServiceScriptConfig)?
There was a problem hiding this comment.
Yes, but then we'd be bypassing the type checker all together. Here I'm trying to prove to the type checker (and hence ourselves) that the 3 different configs can be discriminated.
| ...config, | ||
| state: 'valid', | ||
| extraArgs: undefined, | ||
| dependencies: config.dependencies as Array<Dependency<ScriptConfig>>, |
There was a problem hiding this comment.
Because that property requires validated dependencies, but the type checker still believes they could be unvalidated, because that's checked earlier in a way that doesn't cause the type checker to narrow.
3 participants
Added a
startedpromise property toScriptChildProcess. We'll be re-using this class for services, and we'll need to know when a child process has spawned. We didn't need that exposed before, because all that mattered was the exit code.Added events and logging specific to services.
Refactored the config types so that classes can hold copies of just a subset of the fields of a config, instead of the full object. In watch mode, service child processes will get handed-off across iterations, and potentially across different build graphs, so I wanted to make sure we don't have a memory leak relating to keeping around references to old build graphs (configs hold references to their dependencies, so potentially that keeps the full build graph live).
Added a
ServiceScriptConfigwhich is guaranteed to haveservice:false, and a stubServiceScriptExecutionwhich currently can produce a fingerprint, but doesn't yet actually start/stop the service command.More incremental work towards services.