Ported to CoffeeScript 2

[mitar]

Fixes #264.

[benlavalley]

I make pretty extensive use of meteor-job-collection and implemented these changes in a local package version of it - so far so good.

In addition to instantiating my job collections with 'new', I also seemed to have to do the same for my jobs -- sample of how I am initializing them is below.

Awesome job with the fixes @mitar!

import { Meteor } from 'meteor/meteor';
import { Job } from 'meteor/vsivsi:job-collection';

import processEmailJob from '../functions/processEmailJob.js';

/* eslint-disable */

Meteor.startup(function () {
	new Job.processJobs('myJobQueue', 'sendJobEmail', {
		payload: 5,
		pollInterval: 2500,
		workTimeout: 5000,
	}, function (jobs, cb) {
		let count;
		count = 0;
		jobs.forEach(function (job) {
			job.progress(30, 100);
			count++;
			processEmailJob(job);
			if (count === jobs.length) {
				//console.log(`Jobs -- Queue: ${job.type} -- Finished | Count :${count}`);
				cb();
			}
		});
	});
});

[mitar]

Thanks for testing it out.

new Job(...) was already before in documentation. So this is why I didn't include it in the changelog. But you are right, it worked before without, but now it does not.

Also the following two lines can probably be removed from Job constructor:

    unless @ instanceof JobQueue
      return new JobQueue @root, @type, options..., @worker

But in contrast with similar lines in job collection, they can stay. In job collection (because they are subclasses) they cannot be before super and they cannot really happen anyway, because ES6 classes assure you call them with new (or they fail). This is why I didn't change the code, just a simpler PR.

Anyway, we should then add to the changelog that both Job and JobCollection require new now.

[vsivsi]

@mitar Thanks for the PR. And @benlavalley thanks for trying out the changes. I'll merge this and do a new release when I have some time next week.

[brookback]

Note: we also had to do this, in job_class.coffee:

class JobQueue

  constructor: (@root, @type, options..., @worker) ->
-    unless @ instanceof JobQueue
-      return new JobQueue @root, @type, options..., @worker
    [options, @worker] = optionsHelp options, @worker

...

-  @processJobs: JobQueue
+  @processJobs: (...args) ->
    new JobQueue(...args)

[mitar]

So I didn't have to change the Job class, but I do not know why. Maybe because it was already compiled for me or something? But yes, your changes above look reasonable.

[vsivsi]

@mitar Are you planning to make the change @brookback suggested to your fork?

[mitar]

So my fork just forks this repo, not the Job repository. So we should also fork that and make a change there.

[vsivsi]

@mitar Right. Sorry, just glanced at this and didn't pick up that detail. I expect to merge this and push out a release this week. Thanks for your patience.

[SimonSimCity]

@vsivsi any chance for this to get through by the end of this month? For now I took this PR as submodule into my repo in order to be able to update to Meteor v1.6.1, but this makes the checkout much heavier ...

[vsivsi]

@SimonSimCity I'll do my best. I intended to do it over this past weekend but, well, I'm not 100% in control of my weekends these days... Any work on this package is really, really a spare-time only thing for me right now.

I should note that @mitar has push access to this repo (granted long ago) and can merge this without my help. Only I can publish to Atmosphere, but @mitar can move things along if so desired.

[mitar]

If you are OKing this, then I can do it.

Can you also add me to meteor-job?

[mitar]

Thanks. :-)

[SimonSimCity]

@brookback I don't need the changes you've added here. In fact, if I add it, I get the following error:

TypeError: Job.processJobs is not a constructor
    at JobCollection.processJobs (packages/vsivsi_job-collection/src/shared.coffee:170:31)
    ...

[SimonSimCity]

@mitar I'm now working with this package on the branch you created and I face the following error when trying to create a stub of an instance of new JobCollection(). I've only tried this on the client yet, so it could be it works on the server (but i have my doubts ...).

Any idea which line could cause the problem and how we could fix it?

Job.setDDP must specify a collection name each time if called more than once.
  _setDDPApply@http://localhost:3100/packages/vsivsi_job-collection.js?hash=2ddf47e5fd7eb79cf55dae6d412e4ae79a8446fe:759:109
  setDDP@http://localhost:3100/packages/vsivsi_job-collection.js?hash=2ddf47e5fd7eb79cf55dae6d412e4ae79a8446fe:795:45
  JobCollectionBase@http://localhost:3100/packages/vsivsi_job-collection.js?hash=2ddf47e5fd7eb79cf55dae6d412e4ae79a8446fe:2490:17
  JobCollection@http://localhost:3100/packages/vsivsi_job-collection.js?hash=2ddf47e5fd7eb79cf55dae6d412e4ae79a8446fe:4653:41
  http://localhost:3100/packages/hwillson_stub-collections.js?hash=95ca9f6eedb6e39b19e7096a1bb5e5bac60c727a:62:57
  forEach@[native code]
  stub@http://localhost:3100/packages/hwillson_stub-collections.js?hash=95ca9f6eedb6e39b19e7096a1bb5e5bac60c727a:57:42
  http://localhost:3100/app/app.js?hash=f68b0c8aa174b723dcb6c5f5abc6fb2f39d2b797:9150:25
  callFn@http://localhost:3100/packages/practicalmeteor_mocha-core.js?hash=8beea7367d1e32321cbe94d562492c67d7ddc5d2:4359:25
  run@http://localhost:3100/packages/practicalmeteor_mocha-core.js?hash=8beea7367d1e32321cbe94d562492c67d7ddc5d2:4352:13
  next@http://localhost:3100/packages/practicalmeteor_mocha-core.js?hash=8beea7367d1e32321cbe94d562492c67d7ddc5d2:4698:13
  http://localhost:3100/packages/practicalmeteor_mocha-core.js?hash=8beea7367d1e32321cbe94d562492c67d7ddc5d2:4720:9
  timeslice@http://localhost:3100/packages/practicalmeteor_mocha-core.js?hash=8beea7367d1e32321cbe94d562492c67d7ddc5d2:12688:27

[brookback]

@SimonSimCity

TypeError: Job.processJobs is not a constructor
    at JobCollection.processJobs (packages/vsivsi_job-collection/src/shared.coffee:170:31)
    ...

Isn't that because some code is trying to call Jobs.processJobs() and the original code is referring to JobQueue in that function? So it becomes:

Jobs.processJobs() -> JobQueue() # This is where the error happens, since we're trying to instantiate JobQueue without `new`

where

class JobQueue
   constructor: () ->
      unless this instanceof JobQueue
         return new JobQueue(...)

?

[SimonSimCity]

@brookback I haven't written coffeescript code (in either version) so I can't tell what's right and what's wrong. I've just tried to take the branch @mitar based this PR on and wanted to get my project running. I thought the snippet you provided here was required, but in fact, the PR has changes enough to make it work - this is all I wanted to state here.

By adding the changes you supposed I got the error mentioned - again, this is was tried on the branch this PR is based on. I haven't tried the included repository on it's own.

[mitar]

I do not have time at the moment to look into this. I ran tests and this is how I tested this branch, @SimonSimCity. Can you see if call you are making is not represented in tests?

[rijk]

@mitar , epic work on this so far. I started on this myself because I hadn't noticed the PR, and I've got to say it's pretty darn difficult to get this to work! Really appreciate your efforts and explanations 👍

[gitTerebi]

Great job, anyway we can get this merged with official repo? If not how do pull your changes down locally?

[sebakerckhof]

Why not just port it to modern js?

[rijk]

[sebakerckhof]

Well, I think we can all agree that coffeescript offers little advantages over ES anymore and I've heard that https://github.com/decaffeinate/decaffeinate is working really good these days.

It could have additional advantages such as getting more people on board as maintainers etc.

So I just started out playing around with it. Apparently the output of decaffeinate still needs quite a lot of manual work to have really idiomatic code. But I'm pretty far with the Meteor side of things. The npm-side still needs to be done (although not strictly necessary, since I'd rather just use the npm package instead of having including it as a subrepo): https://github.com/sebakerckhof/meteor-job-collection/tree/modernize

[rijk]

Ah, I didn't know about that package. But still, I don't think it's worth it.

Advantages:

• Less dependencies
• More future-proof
• Possibly getting more maintainers

Disadvantages:

• Costs time
• Risk of regressions
• No functional improvements to the package

[sebakerckhof]

Yeah, I think that pretty much sums it up. With the risk of regressions bothering me the most, since I have no idea how complete the tests are. I wouldn't suggest such a port if this package was still actively maintained, but since it's not it might make sense.

[vsivsi]

See my comment here regarding Coffeescript, etc.

Basically, yeah.

[rijk]

Personally, I like that the package is in Coffeescript. I use both CS and ES6 in my projects, and while I agree that ES6 going forward is the better choice for new projects, I see no need to rewrite existing stuff. Coffeescript is still maintained (things like => are linked to their native counterparts as they are added), it doesn't add a performance penalty, and I personally think CS is often more legible.

[SimonSimCity]

@mitar I can confirm that this PR here is working as-is. The tests are running through and my project can run the jobs and the tests I have against the job-collection library also run through without any outtakes.

@vsivsi I would really like to see this merged soon so I can update to Meteor 1.6.1 without having to copy this repository into mine.

@sebakerckhof I would touch these things rather step-by-step. These are tasks I'd touch and verify before switching to ES6:

• Import npm packagemeteor-job instead of having it as submodule or even move it in one repository
• Rewrite tests written in tinytest in a more common used framework like mocha in order to support coverage (see: https://github.com/meteor/meteor/issues/4518#issuecomment-277353415)
• Get the code-coverage of the current test-basis and evaluate on improving the test-basis

[mitar]

I will try to get this merged soon.

[SimonSimCity]

@mitar Wait a second .. I discovered a failing test which I could first discover after analyzing them ...

One of the tests outputs

****************************************************************************************************
***** The following exception dump is a Normal and Expected part of error handling unit tests: *****
****************************************************************************************************

but the test itself doesn't output an error. One of the following tests fails, which I misinterpreted to be expected. The tinytest UI shows all tests as green, but when I rewrote the tests to mocha, it was red - and it's not related to what testing-framework I use.

[gitTerebi]

I can confirm that running changes locally and adding some "new" keywords here and there and everything runs without any problems.