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

Avoid an error after a one-time scheduled job #21

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jnkien
Copy link

@jnkien jnkien commented Oct 15, 2014

When setting a scheduled job that occurs just one time, the function
was returning an error because next[1] is then undefined and .getTime()
is not defined. Adding a test to bypass this error.

When setting a scheduled job that occurs just one time, the function
was returning an error because next[1] is then undefined and .getTime()
is not defined. Adding a test to bypass this error.
@zol
Copy link
Member

zol commented Oct 15, 2014

@jnkien - thanks for your PR. The code you've changed comes from https://github.com/bunkat/later/blob/master/src/core/settimeout.js, unmodified except for adding the intendedAt parameter. I would prefer to leave the logic in here as is. Can you reproduce the issue in plain later.js? If not, could you please make the fix outside of SyncedCron._laterSetTimeout.

@jnkien
Copy link
Author

jnkien commented Oct 16, 2014

@zol - thank you for your feedback. I understand the fact that you want to keep the logic of included parts of code from later.js as you made it so far.

I investigate the possibility of a fix outside of SyncedCron._laterSetTimeout. Unfortunately, after doing some test and up to my knowledge, it cannot be done and I don't see how to do it because the error is inside a callback of a later.js function.

Then, I reproduced the issue in plain later.js as follows (still under a Meteor project):

Later = Meteor.npmRequire('later');
var schedule = function(parser) {
    var cronDate = '30 12 16 10 * 2014';// just change the first 4 parameters to trigger the job asap
    return parser.cron(cronDate);
};
var s = schedule(Later.parse);
Later.setInterval(function(){
    console.log("This is a one time scheduled job.");
    return true;
}, s);
console.log('One time scheduled job @' + Later.schedule(s).next(1));

I think I can simply use my own fork in my project (it will work but...) or submit a pull request to the later.jsgithub repository. Then afterwards, you could modified your code and recompile the meteor package?

What do you suggest?

Enable adding scheduled jobs on-the-fly. Jobs can be saved and loaded
between server restarts. A job can be run back if the date is passed.
Display the next schedule time after running a job.
@zol
Copy link
Member

zol commented Oct 24, 2014

Hey @jnkien . Your cron format looks incorrect there, see here. You have 6 entries in the format and don't specify hasSeconds.

In any case, please submit your pull request to the later.js repo and I will update to use the new dependency once the author fixes the issue. Thanks!

@dalgard
Copy link

dalgard commented Feb 23, 2015

Huh. Didn't see you pull request, @jnkien. You might take a look at my pull requests here, I think we've set out to achieve the same thing:

#42

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

Successfully merging this pull request may close these issues.

3 participants