Skip to content
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

Using classes to define jobs? #270

Open
IngwiePhoenix opened this issue Nov 8, 2018 · 13 comments
Open

Using classes to define jobs? #270

IngwiePhoenix opened this issue Nov 8, 2018 · 13 comments
Labels

Comments

@IngwiePhoenix
Copy link

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
Copy link
Member

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
Copy link
Author

IngwiePhoenix commented Nov 8, 2018 via email

@mdemblani
Copy link

@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
Copy link
Author

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
Copy link

edy commented Mar 5, 2019

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
Copy link
Contributor

glensc commented Mar 31, 2020

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
Copy link
Contributor

glensc commented Mar 31, 2020

@glensc
Copy link
Contributor

glensc commented Mar 31, 2020

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
Copy link
Member

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
Copy link
Contributor

glensc commented Apr 1, 2020

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
Copy link
Member

evantahler commented Apr 1, 2020

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
Copy link
Contributor

glensc commented Apr 2, 2020

Never link to branches!

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

@glensc
Copy link
Contributor

glensc commented Apr 13, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants