breejs / bree

Bree is a Node.js and JavaScript job task scheduler with worker threads, cron, Date, and human syntax. Built for @ladjs, @forwardemail, @spamscanner, @cabinjs.
https://jobscheduler.net
MIT License
3.06k stars 79 forks source link

Interval parsing issue #23

Closed ChrisEdgington closed 4 years ago

ChrisEdgington commented 4 years ago

Specifying a job interval of every 10 seconds as opposed to 10s results in the following very confusing error:

    this[kPort].postMessage({
                ^

DOMException [DataCloneError]: function(d) {
        return getInstances("next", 1, d, d) !== later.NEVER;
      } could not be cloned.
    at new Worker (internal/worker.js:200:17)
    at Bree.run (/Users/cedgington/repos/personal/temp/express-example/node_modules/bree/index.js:573:28)

You can see this by simply using the example project (as I did) and modify only the interval.

niftylettuce commented 4 years ago

Almost done with the fix

ChrisEdgington commented 4 years ago

LOL - I just was about to post - debugged it to a problem with how you're using later. Guess I'll wait ...

niftylettuce commented 4 years ago

v1.1.26 has been released to npm

try it out and it should work OK for you @ChrisEdgington - let me know otherwise

I published deprecation notice as well for all versions prior

niftylettuce commented 4 years ago

https://github.com/breejs/bree/releases/tag/v1.1.26

ChrisEdgington commented 4 years ago

The parsing and initial run now works - but then Bree exits, which is different behavior than the other syntax.

const taskman = new bree({ jobs: [ { name: 'Worker 1', path: worker_1, interval: 'every 5 seconds' }, { name: 'Worker 2', path: worker_2, interval: 'every 10 seconds' }, ] })

These jobs run once then Bree exits.

-Chris

On Aug 13, 2020, at 9:46 PM, niftylettuce notifications@github.com wrote:

v1.1.26 has been released to npm

try it out and it should work OK for you @ChrisEdgington https://github.com/ChrisEdgington - let me know otherwise

I published deprecation notice as well for all versions prior

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/breejs/bree/issues/23#issuecomment-673830762, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABBR3ZK7GDPWKDKHWTNBZA3SASJRHANCNFSM4P67QKHQ.

niftylettuce commented 4 years ago

Can you share a reproducible test case or repository I can run?

ChrisEdgington commented 4 years ago

const Bree = require('bree');

const bree = new Bree({ jobs: [ { name: 'Exiter', path: worker, interval: 'every 10 seconds' } ] });

function worker() { console.log('I am working ...') }

bree.start();

On Aug 13, 2020, at 9:52 PM, niftylettuce notifications@github.com wrote:

Can you share a reproducible test case or repository I can run?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/breejs/bree/issues/23#issuecomment-673838222, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABBR3ZLRQNHTH6WTG3O3NM3SASKF7ANCNFSM4P67QKHQ.

niftylettuce commented 4 years ago

looking into this, thanks

niftylettuce commented 4 years ago

I see what's wrong, it's a similar issue where it's not checking for isValid

niftylettuce commented 4 years ago

Got the fix, one sec

niftylettuce commented 4 years ago

The core bug all along with this whole issue was this line:

      const schedule = later.schedule(later.parse.text(value));
      if (schedule.isValid()) return schedule;

Instead of returning schedule, we needed to return later.parse.text(value) instead.

niftylettuce commented 4 years ago

v1.1.27 released, try it out and let me know if this works for you @ChrisEdgington

https://github.com/breejs/bree/releases/tag/v1.1.27

I also published deprecation notice for all versions prior.

ChrisEdgington commented 4 years ago

LOL - yes, I had already fixed the problem there - but I just changed it to return schedule.next(1). I was surprised by your initial fix and decided, “Wow, I must not understand what is going on."

On Aug 13, 2020, at 10:19 PM, niftylettuce notifications@github.com wrote:

The core bug all along with this whole issue was this line:

  const schedule = later.schedule(later.parse.text(value));
  if (schedule.isValid()) return schedule;

Instead of returning schedule, we needed to return later.parse.text(value) instead.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/breejs/bree/issues/23#issuecomment-673846413, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABBR3ZKSFF525JD7Y76SZH3SASNKHANCNFSM4P67QKHQ.

niftylettuce commented 4 years ago

🤦 sorry hahahaha

ChrisEdgington commented 4 years ago

1.1.27 appears to be working properly - thanks for the fix.

On Aug 13, 2020, at 10:23 PM, niftylettuce notifications@github.com wrote:

🤦 sorry hahahaha

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/breejs/bree/issues/23#issuecomment-673847544, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABBR3ZLCUNKK4BPJNJEIT7LSASN37ANCNFSM4P67QKHQ.

niftylettuce commented 4 years ago

Great to hear, thank you @ChrisEdgington for confirmation. If you need anything else or have questions let me know.