Conversation
aomarks
commented
Oct 21, 2022
| if (this._shouldNotStart) { | ||
| return {ok: false, error: [this._startCancelledEvent]}; | ||
| } | ||
| try { |
Member
Author
There was a problem hiding this comment.
Note this is not a very well formatted diff. It's all indentation inside the try.
justinfagnani
approved these changes
Oct 21, 2022
| config, | ||
| this, | ||
| this._logger, | ||
| this._abort.promise |
There was a problem hiding this comment.
Why aren't you using an AbortSignal?
Member
Author
There was a problem hiding this comment.
Because it's only supported in Node 15+, and we still support Node 14 until it goes out of LTS next year. Simpler than having a polyfill since it's so trivial anyway.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a
servicesNotNeededpromise, which gets resolved when a script either knows it will never run, or when it has finished running, and hence no longer needs the services it depends on to be running. This is how services will keep track of how many dependents still need them to be running -- once there are none left, they can shut down (unless they are directly invoked).This is done via a new
BaseExecutionWithCommandbase class, which is only relevant to scripts with commands (because no-command scripts don't directly consume services).Also:
Pass
abortpromise down to services. Directly-invoked services will need this to know when to shut down on SIGINT.Removed a redundant
throwI noticed.Part of #33