cujojs / when

A solid, fast Promises/A+ and when() implementation, plus other async goodies.
Other
3.44k stars 396 forks source link

Make when.js compatible with Vertx 2 #183

Closed mohlemeyer closed 11 years ago

mohlemeyer commented 11 years ago

The JavaScript language module of Vertx 2, which is based on a newer version of Rhino, provides a CommonJS module system. There is no free global vertx variable and by using when.js as a CommonJS module, such a global variable cannot be introduced for when.js. As a result, since Vertx 2 also does not provide a global setTimeout function, the nextTick function cannot be set, causing when.js to fail.

To still use when.js on Vertx 2 (and keep it compatible with older Vertx versions) I did the following.

    try {
        var vertx2 = require ? require('vertx') : undefined;
    } catch (ignore) {}
...
    } else if (typeof vertx2 === 'object') {
        nextTick = vertx2.runOnContext;
    } else if (typeof vertx === 'object') {
        nextTick = vertx.runOnLoop;
    } else {
...

Maybe something to consider for inclusion to when.js?

briancavalier commented 11 years ago

Hey @mohlemeyer, we'd love to support vertx2. I can tell you that when.js 3.0 will provide a way to configure your own scheduler, as well as other things. If you're interested to check that out, have a look at the when3-proto branch. I'm on holiday until later in August, but I will definitely look at this when I get back. Thanks!

mohlemeyer commented 11 years ago

Well, it seems that my solution for when.js 2.3 did only cover the "nextTick" case, but there are more vertx dependencies in timeout.js and delay.js, which I overlooked because I currently do not need the methods provided by those files..

I had a look at the when3-proto branch, but could not figure out how to configure "my own" scheduler, but it seems that the vertx dependencies are now located in the two files lib/timer.js and lib/makeScheduler.js. And actually I would prefer vertx2 to be supported out of the box without the need to configure a special scheduler.

I think that the solution above could be applied similarly to both files to achieve vertx2 compatibility.

briancavalier commented 11 years ago

there are more vertx dependencies in timeout.js and delay.js

Yeah, they need the vertx2 equivalents of setTimeout/clearTimeout. Have those changed in vertx2, or are they still vertx2.setTimer and vertx2.cancelTimer. If so, can you point me to docs for the correct functions to use for timers?

in the two files lib/timer.js and lib/makeScheduler.js

Yep, those are the two. We can probably just add sniffs for vertx2 in those two, and when3 will just work.

Let me know about the timer functions, and we may be able to get something into the 2.x line.

mohlemeyer commented 11 years ago

Yeah, they need the vertx2 equivalents of setTimeout/clearTimeout. Have those changed in vertx2, or are they still vertx2.setTimer and vertx2.cancelTimer. If so, can you point me to docs for the correct functions to use for timers?

Those functions remained the same (see docs here). The difference is, that no "global" vertx or vertx2 object exists - your have to require it. So in both files, immediately after the line when = require('./when'); you could to something like

if (typeof vertx !== 'object') {
    try {
        this.vertx = require('vertx');
    } catch (ignore) {}
}

It seems that both files will only be loaded in module environments, so there should be no danger of cluttering the global namespace by assigning to this.vertx. This approach would not work for the when.js file, since you have to distinguish between vertx 1 and vertx 2.

mohlemeyer commented 11 years ago

Yep, those are the two. We can probably just add sniffs for vertx2 in those two, and when3 will just work.

Sorry, I haven't found a way to sniff for vertx2. But using the approach described in my last comment for timer.js and the approach from my first comment for makeScheduler.js should work.

briancavalier commented 11 years ago

Cool, thanks for all the info. I'll see about using those two techniques in the when 2.x line, and also porting them forward to when3. We don't have an official release timeline for 3.0 yet, so hopefully we can get this into a 2.x release.

briancavalier commented 11 years ago

Quick q: does require('vertx') work in vertx 1.x or only in 2.x?

briancavalier commented 11 years ago

Leaning on require('vertx') actually seems to work in both 1.x and 2.0.1:

try {
    // vert.x 1.x || 2.x
    nextTick = require('vertx').runOnLoop || require('vertx').runOnContext;
} catch(ignore) {
    nextTick = function(t) { setTimeout(t, 0); };
}

I kind of like that since it ignores the vertx global entirely, even in 1.x. Does that seem ok to you?

briancavalier commented 11 years ago

Ok, I think I have something working! Can you try out the vertx2 branch and let me know if it works for you? It includes support for both the core when.js module as well as the delay and timeout modules. In my very minimal testing :), it seems to work in both vertx 1.3.1 and 2.0.1.

mohlemeyer commented 11 years ago

OK, I'll be trying that over the weekend. I wonder why require works for 1.3.1, the docs for that version still mention load as the (only) mechanism to load other scripts. Maybe it went in as part of a Rhino upgrade to 1.7R4 somewhere between 1.0 and 1.3.1?

briancavalier commented 11 years ago

Yeah, seems like it was added somewhere around 1.1.0 via eclipse/vert.x#133, which was a year ago. Seems safe enough to go with.

Let me know how it goes, and I'll be sure to add a note in the changelog about supporting vertx >= 1.1.0.

mohlemeyer commented 11 years ago

when.js now works without modifications for me. I did some additional tests for delay and timeout and they ran as expected. Thanks for all your effort!

briancavalier commented 11 years ago

@mohlemeyer Awesome, thanks for verifying. I just merged into the dev branch, so it can go out with the next release (soon, probably within the week).

We don't have unit test coverage on vert.x, so we can't claim official support--but at least we can say that it works, and if people find problems we can likely fix them. On that note, is there a vertx unit testing tool you recommend? We use BusterJS for other platforms, and have been very happy with it, but afaik, it doesn't support vertx.

Closing this, but if you find any problems in the meantime, just let me know! Thanks for the help!

mohlemeyer commented 11 years ago

I haven't found a JavaScript testing tool to work out of the box on vertx, but I managed to get QUnit and Sinon.JS working. The nice thing about these two is, that they do not have any other node specific dependencies, which made porting to vertx feasible. Combined with a homebrew QUnit-Plugin for JUnit compatible test output, a small testrunner and a matching ant-Task, test automation not works for my needs. As I find time to remove a few rough edges I might ask the vertx community, if there is any wider interest in such a tool. I don't think it would help to get when.js unit tested for vertx because BusterJS tests and QUnit tests are probably not compatible.

briancavalier commented 11 years ago

Sounds cool. I'm sure the vert.x community would benefit from it! We're big fans of Sinon.JS--it comes bundled with BusterJS.

You're probably right in that Buster and QUnit aren't compatible, tho. If the time comes that when.js is used more widely on vert.x, we'll need to figure out what to do. Our policy in cujoJS is only to "officially" claim to support a platform if we have unit test coverage. I'm sure we can figure out a solution.