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

Invocable arguments? #128

Closed
lloydjatkinson opened this issue Nov 9, 2019 · 9 comments
Closed

Invocable arguments? #128

lloydjatkinson opened this issue Nov 9, 2019 · 9 comments

Comments

@lloydjatkinson
Copy link

lloydjatkinson commented Nov 9, 2019

Hi,

Not sure if this is more of a feature request or a question but anyway. With Quartz.NET and Hangfire, arguments can be passed to jobs. (https://www.quartz-scheduler.net/documentation/quartz-2.x/tutorial/more-about-jobs.html)

How would we achieve something similar to this with Coravel? A use case for this would be a "daily email report" type job, except the invocable is registered twice but with a different argument, perhaps indicating which mailing list to use, and one mailing list is for 8AM while the other is 9AM.

@jamesmh
Copy link
Owner

jamesmh commented Nov 10, 2019

Off the top of my head....

services.AddTransient<DailyEmailReportInvocable>(p => new DailyEmailReportInvocable("8am"));
services.AddTransient<DailyEmailReportInvocable>(p => new DailyEmailReportInvocable("9am"));

Something like that. You can instruct the service container to use those lambda methods when instantiating your invocable classes. You can inject whatever you want (like you were asking for).

If you need access to another service from DI, you can do something like:

services.AddTransient<DailyEmailReportInvocable>(p => new DailyEmailReportInvocable(p.GetService<SomeOtherType>(), "8am"));

Again, off the top of my head. Might be some slight variation on the real method names etc. 🙃

Now, since you are assigning the same class twice, the scheduler, for example, would only pick up one of those when using scheduler.Schedule<MyInvocable>().

So you could do:

scheduler.ScheduleAsync(async () => {
    var job = new SomeInvocable("MailList1");
    await job.Invoke();
}).DailyAtHour(8);

scheduler.ScheduleAsync(async () => {
    var job = new SomeInvocable("MailList2");
    await job.Invoke();
}).DailyAtHour(9);

@jamesmh jamesmh closed this as completed Nov 10, 2019
@jamesmh
Copy link
Owner

jamesmh commented Nov 10, 2019

You can take a look at this sample:

https://github.com/jamesmh/coravel/blob/master/Samples/QueueWithCancellationTokens/Controllers/QueueController.cs

That's queuing an invocable class.

Let me know if you have any questions?

@jamesmh jamesmh reopened this Nov 10, 2019
@jamesmh
Copy link
Owner

jamesmh commented Nov 10, 2019

So, this suggestion I made:

scheduler.ScheduleAsync(async () => {
    var job = new SomeInvocable("MailList1");
    await job.Invoke();
}).DailyAtHour(8);

Potentially, I could add a new method on the scheduler:

scheduler
    .ScheduleWithParams<SomeInvocable>("MailList1")
   .DailyAtHour(8);

And that would pass the parameters into the Invocable via the contructor... or even use property injection (which might work better with expressions...).

Anyways, this would make a nice addition like you suggested 👌

@lloydjatkinson
Copy link
Author

Thank you for taking a look :)

@schmitch
Copy link

actually passing it via the instructor wouldn't be that good, since it will make it harder to register the invocable.

@jamesmh
Copy link
Owner

jamesmh commented Dec 31, 2019

FYI I'm thinking of preferring a property injection type approach.

So instead of this:

scheduler
    .ScheduleWithParams<SomeInvocable>("MailList1")
   .DailyAtHour(8);

It would be like this:

scheduler
    .ScheduleWithParams<SomeInvocable>(i => i.MailListName = "MailList1")
   .DailyAtHour(8);

Why not constructor injection?

Because invocables are already configured to work via the DI container with .NET Core.

Using the constructor injection method means that invocables cannot use the DI container anymore (since Coravel now needs to manually instantiate this invocable).

With property injection, or at least allowing an extra feature that set whatever properties you need, avoids this issue and you gain the best of both worlds.

EDIT

Actually, there is a way to inject from DI and from user defined properties (which @Blinke has implemented 😊)

I still think allowing flexibility in potentially having an invocable that you can use with params and without (via property type injection) is "better".

Open to thoughts of course! What does everyone think?

@lloydjatkinson
Copy link
Author

Constructor injection is the best way.

@jamesmh
Copy link
Owner

jamesmh commented Dec 31, 2019

Why do you think so @lloydjatkinson ?

@jamesmh
Copy link
Owner

jamesmh commented Feb 21, 2020

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

No branches or pull requests

3 participants