Jobs - #12970
Conversation
d680391 to
efb33cb
Compare
7e99cd1 to
fe37416
Compare
| const yarnInstall = jobs.createJob(/* ... */, { name: 'yarn-install', extends: dispatcher }); | ||
| const pnpmInstall = jobs.createJob(/* ... */, { name: 'pnpm-install', extends: dispatcher }); | ||
|
|
||
| declare const registry: jobs.SimpleJobRegistry; |
There was a problem hiding this comment.
declare here is not correct, since you are not writing a declaration file.
There was a problem hiding this comment.
This is an example in the README. It's meant to provide context on the type, not be copy-pasted as is.
| # Jobs | ||
|
|
||
| Jobs is the Angular DevKit subsystem for scheduling and running generic functions with clearly | ||
| typed inputs and outputs. A Job is |
There was a problem hiding this comment.
"A Job is" is not a sentence, would ❤️ to see a longer definition here.
There was a problem hiding this comment.
Clarifying this. I'm reworking the README a bit and by EOD I'll have a proper description to replace that non-sentence.
| import { jobs, schema } from '../src'; | ||
| import { ModuleNotFoundException, resolve } from './resolve'; | ||
|
|
||
| export class NodeModuleJobScheduler extends jobs.SimpleScheduler { |
There was a problem hiding this comment.
What do you think about making a JobRegistry interface that a scheduler would then use. Job discovery and job scheduling are really orthogonal tasks. Subclassing a scheduler wouldn't typically be needed then and being able to compose registries would provide for interesting behavioral options as well.
| }); | ||
|
|
||
| // Extending makes sure the input and output matches the dispatcher. | ||
| const npmInstall = jobs.createJob(/* ... */, { name: 'npm-install', extends: dispatcher }); |
There was a problem hiding this comment.
extends feels wrong semantically. implements? handles? provides?
Actually, is this option even needed? The dispatcher is the element that requires the knowledge of the connection between the new job and itself.
There was a problem hiding this comment.
Yeah I'm starting to think that extends as a concept is specific to the registry and should not be part of the job declaration. It was deeper integrated with the API and then I moved it slowly out of it. I think I should remove it entirely (and keep it as a field in the register() functions).
There was a problem hiding this comment.
Going to go with implements, as it's more an interface related concept.
| * Metadata associated with a job. | ||
| */ | ||
| export interface JobDescription extends JsonObject { | ||
| readonly name: JobName; |
There was a problem hiding this comment.
I'm naming this as a hint that everywhere that type is used should be the job name itself and not just a random string (like a message). This is used as a hint ot users, not actually typing.
| return s; | ||
| }), | ||
| inputChannel: inputChannel.asObservable(), | ||
| logger, |
There was a problem hiding this comment.
Shouldn't the logger come from the context?
There was a problem hiding this comment.
There is no logger in the Job interface, only a JobEvent kind of Log. The context passed in the createJob handler is expanded, there's no logger in the Job api by default (and by design)
filipesilva
left a comment
There was a problem hiding this comment.
Initial review of the readme.
Reading through the description, Jobs could also find a home in the benchmark tool. The benchmark tool is doing very similar things with scheduling individual runs, including the multiple output observables.
| 1. `id`. A unique symbol that can be used as a Map key. | ||
| 1. `description`. The description of the job from the scheduler. See `JobDescription` object. | ||
| 1. `input`. An `Observer` that can be used to send validated inputs to the Job itself. | ||
| 1. `output`. An `Observable<OutputType>` that filters out events to get only the returned output |
There was a problem hiding this comment.
What does filters out events to get only the returned output mean? I don't understand which events these are.
| Jobs can be scheduled using a `Scheduler` interface, which contains a `schedule()` method. The | ||
| method takes a name, an input and a list of options. | ||
|
|
||
| ## Synchronizing and Dependencies |
There was a problem hiding this comment.
Isn't synchronization managed by the execution strategy?
Are these different things, and if so, what are the different use cases?
There was a problem hiding this comment.
These are different things. The difference is that strategy is set by the actual Job code, while this synchronization is done by the person scheudling the job. Clarfiied.
|
Sorry guys I've pushed master to this branch instead of pulled my branch to my computer this morning. This is going to be one of those days... 😩 |
|
Moved to #13029. |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
There is a README added which contains how to use the API.