actionhero / node-resque

Node.js Background jobs backed by redis.
https://node-resque.actionherojs.com
Apache License 2.0
1.37k stars 151 forks source link

Using classes to define jobs? #270

Open IngwiePhoenix opened 5 years ago

IngwiePhoenix commented 5 years ago

I would like to know if it is possible to create a job as a class. For instance, would something like this be possible?

class SendNotificationMails extends someBaseJobClass {
  constructor() {
    this.attach("notification-mails", /*... other settings */)
  }
  async run() {
    // Execute job...
  }
}

It would make organizing a lot of background jobs much easier to write everything in classes and just have them register at an initial script.

evantahler commented 5 years ago

I agree! I would recommend an approach that if the perform method is a function, it runs it, or if it is a Class, it would exec a new instance.

Do you need help writing a Pull Request?

IngwiePhoenix commented 5 years ago

I am in fact incredibly rusty in coding - spend two years in hospital, I am just really getting back. Bit by bit…or byte.

I will try to see if I can do something there, but I wouldn’t be looking forward for that too much… So if you can, could you try to put something together there?

Am 08.11.2018 um 19:53 schrieb Evan Tahler notifications@github.com:

I agree! I would recommend an approach that if the perform method is a function, it runs it, or if it is a Class, it would exec a new instance.

Do you need help writing a Pull Request?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/taskrabbit/node-resque/issues/270#issuecomment-437114110, or mute the thread https://github.com/notifications/unsubscribe-auth/ACwnlETZFxT-WbjMmL1PWO6Dk8_whjwHks5utH2MgaJpZM4YVNQE.

mdemblani commented 5 years ago

@IngwiePhoenix It is quite possible and I am in-fact doing that. As you wrote above, a simple base-class which simply confirms the contract, along with also providing simple details such as the name of the job and handle function which handles the job. All these classes are present in a designated folder.

You can then utilise the fs npm library, to load all the classes from the folder with a few exceptions. Then when setting up the tasks, you can pass the reference to the handle function as the task to be performed.

IngwiePhoenix commented 5 years ago

Definitively! I have kind of lost track of what I had started on in this PR. I may just re-write the PR again and put my mind to it :) I am very sorry I went silent on this...

edy commented 5 years ago

This is how i do my jobs.

class BaseJob {
    async perform () {
        throw new Error('You must specify the perform() method.');
    }
}

BaseJob.attachJob = function startJob (JobClass) {
    // this function is called by node resque
    return async () => {
        const j = new JobClass();
        try {
            const performResult = await j.perform();
            // maybe do something here
            return performResult;
        } catch (e) {
            j.error(e);
            throw e;
        }
    };
};

class AlwaysFail extends BaseJob {
    async perform () {
        this.log('Starting failing job.');

        await this.doStuff();

        this.log('Ending failing job.');
        throw new Error('I failed my job');
    }

    async doStuff () {
        this.log('Doing my stuff...');
        return await new Promise((r) => setTimeout(r, 1000));
    }
}

module.exports = {
    name: 'Always fail',
    // plugins: [],
    // pluginOptions: {},
    perform: BaseJob.attachJob(AlwaysFail),
};
glensc commented 4 years ago

I did like this

export class Job {
  async perform(): Promise<number> {
    console.log(this);
    const cmd = this.createCommand();
    return 0;
  }

  private createCommand() {

  }
}

  const jobs = {
    job: new Job(),
  };

  worker = new Worker(
    { connection: connectionDetails, queues: ["test"] },
    jobs
  );

this worked fine, until I needed to call this. in the perform() method, but that fails as method is calling perform has this pointing to Worker object

glensc commented 4 years ago

the this, is passed here:

glensc commented 4 years ago

I ended up creating this wrapper:

/**
 *
 * Wrapper to overcome issue that perform is called with wrong this context.
 *
 * @author Elan Ruusamäe <glen@pld-linux.org>
 *
 * https://github.com/actionhero/node-resque/issues/270
 */
class JobWrapper {
  static wrap(proxy: any) {
    return {
      perform: async (...args: []) => {
        return await proxy.perform.apply(proxy, args);
      }
    };
  }
}

  const jobs = {
    test: JobWrapper.wrap(new Job()),
  };
evantahler commented 4 years ago

Nice!

I would also be open to changing application of this and just passing the worker as an argument to the job. It would be a breaking change, but likely would make node-resque easier to use.

glensc commented 4 years ago

I think you can make it non-breaking change (or minimal impact affecting only class uses) if you test is each jobs key is a class instance (new foo) rather pure javascript object ({}) and change behavior if it's class.

I'm not sure is that distinguishing possible, but probably small StackOverflow search will give the answer.

evantahler commented 4 years ago

If you are looking for another example of a wrapping method, here's how Actionhero takes it's Task classes and structures them into the node-resque methods https://github.com/actionhero/actionhero/blob/3f9d936000d44afffd8798f1c3f5a3cb872390d2/src/initializers/tasks.ts#L62-L134

glensc commented 4 years ago

Never link to branches!

I've already encountered two of your broken links when getting acquainted with this project.

glensc commented 4 years ago

@evantahler fix your post (edit it) and use permalink: