-
Notifications
You must be signed in to change notification settings - Fork 125
Services: Config types, events, stub execution class, and started promise #461
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
Changes from all commits
cb390e0
30d1937
445ce31
f30eca2
9860df0
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 |
|---|---|---|
|
|
@@ -1082,19 +1082,44 @@ export class Analyzer { | |
| }; | ||
| return {ok: false, error: this._markAsInvalid(config, failure)}; | ||
| } | ||
| { | ||
| const validConfig: ScriptConfig = { | ||
|
|
||
| let validConfig: ScriptConfig; | ||
| if (config.service) { | ||
| // We should already have created an invalid script at this point, so we | ||
| // should never get here. We throw here to convince TypeScript that this | ||
| // is guaranteed. | ||
| if (config.command === undefined) { | ||
| throw new Error( | ||
| 'Internal error: Supposedly valid service did not have command' | ||
| ); | ||
| } | ||
| validConfig = { | ||
| ...config, | ||
| state: 'valid', | ||
| extraArgs: undefined, | ||
| dependencies: config.dependencies as Array<Dependency<ScriptConfig>>, | ||
| // Unfortunately TypeScript doesn't narrow the ...config spread, so we | ||
| // have to assign explicitly. | ||
| command: config.command, | ||
| }; | ||
| } else { | ||
| validConfig = { | ||
| ...config, | ||
| state: 'valid', | ||
| extraArgs: undefined, | ||
| dependencies: config.dependencies as Array<Dependency<ScriptConfig>>, | ||
| // Unfortunately TypeScript doesn't narrow the ...config spread, so we | ||
| // have to assign explicitly. | ||
| service: config.service, | ||
|
Collaborator
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. Can you cast the config:
Member
Author
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. 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. |
||
| }; | ||
| // We want to keep the original reference, but get type checking that | ||
| // the only difference between a ScriptConfig and a | ||
| // LocallyValidScriptConfig is that the state is 'valid' and the | ||
| // dependencies are also valid, which we confirmed above. | ||
| Object.assign(config, validConfig); | ||
| } | ||
|
|
||
| // We want to keep the original reference, but get type checking that | ||
| // the only difference between a ScriptConfig and a | ||
| // LocallyValidScriptConfig is that the state is 'valid' and the | ||
| // dependencies are also valid, which we confirmed above. | ||
| Object.assign(config, validConfig); | ||
|
|
||
| return {ok: true, value: config as unknown as ScriptConfig}; | ||
| } | ||
|
|
||
|
|
||
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.
Why was this cast needed?
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.
Because that property requires
validateddependencies, but the type checker still believes they could beunvalidated, because that's checked earlier in a way that doesn't cause the type checker to narrow.