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.

[SimonSimCity]

@mitar I give you green light again🚦

The test failing in the test-collection was also failing at exactly the same condition in the current release (v1.5.2).

[holmrenser]

Any idea when this will be merged?

[SimonSimCity]

@mitar @vsivsi Any update ..? I currently consider publishing a package myself because I want to update to Meteor 1.7.0 when it's released (which could be any day soon).

@vsivsi What is missing on this PR to be a high-quality PR? Or have you simply not had the time to take a deep look at it? Because this is now going on for quite some months now and is a show-stopper - not just a feature request.

[SimonSimCity]

I've now taken this into hands and published my fork on atmosphere, which contains this fork and both of my other open PRs on this repository.

I'm currently looking into how to refactor the code and your approach @sebakerckhof is something I'm analyzing very closely and I'd like to push it in this direction because it could majorly change how many of the community are contributing. This could even open for splitting this package into a core and a worker part, where both packages are not bound together, but you'd have to add them one-by-one.

The reason that the tests of this repository didn't grow with the features added makes it quite hard for me to determine how much you've done - mostly because it's all in one commit. I'd very much like to see your contribution on my fork.

[SimonSimCity]

@sebakerckhof I found out something strange: When I use the code generated by https://github.com/decaffeinate/decaffeinate, the tests give a different result. This is also the reason why I now work on code generated by calling coffee -c on each file, which also is the exact output of the coffee-script parser.

Feel free to follow my project: https://github.com/simonsimcity/meteor-job-collection I'll do everything in small incremental steps as I go.

[SimonSimCity]

For all interested 😊 https://github.com/decaffeinate/decaffeinate/issues/1313

[tcastelli]

would be nice to get this merged when possible :+1:

[arggh]

Would be awesome to get this great package working with Meteor v1.6. @vsivsi do you have any estimates on when this package could possibly be Meteor 1.6/1.7 compatible, if ever?

Is there something that needs to be done regarding this PR to get it merged?

@SimonSimCity is your fork running smoothly in any real apps?

[SimonSimCity]

@arggh Well, it's working well in my projects ... As of https://atmospherejs.com/simonsimcity/job-collection it's not very widely used yet.

I've not done much in the latest release excepted for updated to CoffeeScript 2.0. I was able to yet kick out CoffeeScript completely and got all tests to pass that passed before, but I still have goals I work on.

I also encourage everyone to try my solution and to open issues and PRs. I have a personal plan I'd like to go for which fit's my project, but this is something to talk about.

[SimonSimCity]

@vsivsi I highly respect all the effort you've put into this project and also do respect your choice to put this project into maintenance mode.

What I don't like so far is that there are many people still using this project who now are stuck because they have to choose between staying on your or on my branch. I'm very much open to merge my changes back if you give me an option to publish a new version myself or you promise to be available for this.

I have to move on with this project and keep it compatible as my company is relying on it. I'll also keep improving it. If I don't have anyone to talk to, I'll move it into a direction where I see it being flexible and compatible enough to stand future Meteor releases.

[gitTerebi]

@SimonSimCity Ill be happy to start trying your release and report any bugs as it appears. So far I'm using a purely local only copy monkey patched to work with CS 2.0.

But getting rid of CS entirely would be awesome!

[lmachens]

@SimonSimCity I am using your release for a month now. No issues so far.

[mitar]

I would suggest to move this repository to https://github.com/Meteor-Community-Packages and at the same time give ownership of the repository also to @SimonSimCity to (help) maintain it.

@vsivsi would that be OK with you?

[SimonSimCity]

:+1: If this happens, I'll publish a new version of my fork printing out a console.log() message warning the user that my fork has been merged back again and will not be maintained anymore in favor to this project.

I'm not very deep into the code-base, but I can maintain it as good as I know it.

[mitar]

Awesome, thanks @SimonSimCity.

Let's wait a bit for @vsivsi, if he agrees (I do not have adminship of this repository for it to be moved). I do think it would be the best to move it instead of making a fork.

[vsivsi]

@mitar As I indicated on the other issue, I'd be happy to move this package over.

[mitar]

Awesome. Then feel free to move it to my personal account everything you think is reasonable and I will move it over to community packages.

We want to retain all package names as much as possible, so getting permissions on them so that we can publish all packages would also be great. We have a communitypackages Meteor organization and @meteor-community NPM group to be added to all packages.