Added Scheduled Commands #1

Merged
merged 11 commits into from Oct 20, 2016

Projects

None yet

2 participants

@saverio
Contributor
saverio commented Oct 17, 2016

Added a new built in command called schedule:run that looks for registered app commands that have a schedule() function and attempts to schedule them to run.

Scheduling a command is as easy as adding a schedule() method to your command. It should look something like this:

schedule() {
    return this.scheduler.everyMinute()
}
Saverio Mond... added some commits Oct 17, 2016
src/Command.js
+ }
+
+ execAsChildProcess() {
+ const exec = require('child_process').exec
@shnhrrsn
shnhrrsn Oct 17, 2016 edited Member

Should be at the top of the file as import ChildProcess from 'child_process' and then used via ChildProcess.exec

src/Command.js
+ env: process.env
+ }
+
+ return new Promise(async (resolve, reject) => {
@shnhrrsn
shnhrrsn Oct 17, 2016 edited Member

Using async/await here is unnecessary given that you’re already returning a promise

src/ScheduleCommand.js
+ const scheduled = []
+
+ for(var command of this.cli.commands) {
+ if(typeof(command.schedule) === 'function') {
@shnhrrsn
shnhrrsn Oct 17, 2016 Member

typeof doesn’t need to be a function, just typeof command.schedule === 'function' will suffice

src/Scheduler.js
+ start(job) {
+ return new Promise((resolve, reject) => {
+ if(!this.schedule) {
+ reject('No schedule has been defined.')
@shnhrrsn
shnhrrsn Oct 17, 2016 Member

reject should return to avoid code processing below

src/Command.js
@@ -173,6 +176,43 @@ export class Command {
})
}
+ schedule() {
+ return Promise.reject('Command does not implement schedule(): ' + this.name)
@shnhrrsn
shnhrrsn Oct 17, 2016 Member

I’m thinking it makes the most sense to explicitly register scheduled commands in a provider (CommandProvider likely).

Two big feature gaps I see:

  1. Currently can’t run the same command multiple times (just at different intervals)
  2. No way to pass in cli arguments/options

Laravel handles this pretty cleanly:

$schedule->command('emails:send --force')->daily();
$schedule->command(EmailsCommand::class, ['--force'])->daily();
src/Command.js
+
+ registerSchedule() {
+ return new Promise((resolve, reject) => {
+ this.schedule().then((scheduler) => {
@shnhrrsn
shnhrrsn Oct 17, 2016 Member

Looks like this.schedule() returns a promise, so wrapping this in new Promise is unnecessary, can just return the promise form this.schedule()

src/ScheduleCommand.js
+
+ const scheduled = []
+
+ for(let command of this.cli.commands) {
@shnhrrsn
shnhrrsn Oct 17, 2016 Member

command.schedule should also be a function now that it’s been added to the base Command class, so this can be simplified to:

const scheduled = this.cli.commands.map(command => command.registerSchedule())
src/ScheduleCommand.js
+ }
+ }
+
+ await Promise.all(scheduled)
@shnhrrsn
shnhrrsn Oct 17, 2016 Member

async/await is unnecessary here, just return Promise.all(scheduled)

Saverio Mondelli Code review changes. dec8a77
src/ScheduleCommand.js
@@ -0,0 +1,22 @@
+import './Command'
+
+export class ScheduleCommand extends Command {
@shnhrrsn
shnhrrsn Oct 17, 2016 Member

Rename to ScheduleRunCommand to match the command’s name

+
+ start() {
+ return new Promise((resolve, reject) => {
+ this.jobs.map((job) => {
@shnhrrsn
shnhrrsn Oct 19, 2016 Member

Looks like there’s a bug here that could cause reject to fire multiple times… Let’s refactor to:

for(const job of jobs) {
    if(!job.schedule) {
        return reject('Job is missing a schedule.')
    }

    job.start()
}
+ }
+
+ create(value, args) {
+ if(typeof value === 'function' && value.name.endsWith('Command')) {
@shnhrrsn
shnhrrsn Oct 19, 2016 Member

I think value instanceof Command makes sense here over endsWith

+ let cmd = [ ]
+
+ if(name.indexOf(' ') > 0) {
+ cmd = name.trim().split(' ')
@shnhrrsn
shnhrrsn Oct 19, 2016 Member

Let’s do .split(/\s+/) to be safe

+ return this
+ }
+
+ localtime() {
@shnhrrsn
shnhrrsn Oct 19, 2016 Member

Rename to localTime

@@ -173,6 +174,32 @@ export class Command {
})
}
+ execAsChildProcess(args) {
@shnhrrsn
shnhrrsn Oct 19, 2016 Member
execAsChildProcess(args = null)
+ env: process.env
+ }
+
+ if(!args) {
@shnhrrsn
shnhrrsn Oct 19, 2016 Member
if(args.isNil)
+
+ return new Promise((resolve, reject) => {
+ execFile(process.env.CLI_BIN, args, options, (err, stdout, stderr) => {
+ if(err instanceof Error) {
@shnhrrsn
shnhrrsn Oct 19, 2016 Member

This can just be:

if(!err.isNil)
+ run() {
+ this.info('Schedule daemon starting...')
+
+ this.cli.scheduler.start()
@shnhrrsn
shnhrrsn Oct 19, 2016 Member

Let's refactor to:

return this.cli.scheduler.start().then(() => new Promise(() => { }))

Scheduler.start can fail, so it makes sense to at least surface those errors and abort before we keep it open

@shnhrrsn
shnhrrsn Oct 19, 2016 Member

Actually it looks like Scheduler.start will never resolve if there's no error, so we can just do:

return this.cli.scheduler.start()
@saverio saverio merged commit 03f2c69 into grindjs:master Oct 20, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment