eclipse-vertx / vert.x

Vert.x is a tool-kit for building reactive applications on the JVM
http://vertx.io
Other
14.32k stars 2.08k forks source link

Provide setImmediate #448

Closed kriskowal closed 11 years ago

kriskowal commented 11 years ago

setImmediate is a W3C proposal for efficiently scheduling an event and yielding to the event loop. If vert.x were to implement this or Node’s process.nextTick, cross-engine libraries like Q would be able to work without modification.

pidster commented 11 years ago

Does runOnLoop() work for your requirement: http://vertx.io/api/java/api/org/vertx/java/core/Vertx.html#runOnLoop(org.vertx.java.core.Handler)

kriskowal commented 11 years ago

No. Cross engine libraries can expect to run in browsers or with asynchronous CommonJS module loaders. The best solution is for all engines to implement the standard scheduler.

Feature detecting the presence of the Java packages name-space is a non-starter for any code that runs in browsers because touching that namespace initializes the JVM, whether it is used or not. Using a require in a try-catch is also a non-starter because require calls are statically analyzed by asynchronous module loaders. The require call would be a false-positive for dependency analyzers in other engines.

blalor commented 11 years ago

I agree that Vert.x should eagerly implement standards (even emerging ones), but the real problem (in my opinion) is that Rhino isn't providing these basic mechanisms. With respect to this particular issue, however, Vert.x's JavaScript runtime support is only provided after load()'ing or require()'ing vertx.js. It's not in the global namespace, and I wasn't able to find a way to provide something equivalent to setTimeout from the script that require()s Q.

pidster commented 11 years ago

So it's OK for vert.x to implement another engine's API but not for the cross-engine library to handle that?

Each language implementation in vert.x has a functionally equivalent method, so you can use the same mechanism in whichever language you want:

https://github.com/vert-x/vert.x/blob/master/vertx-lang/vertx-lang-rhino/src/main/javascript_scripts/core/timers.js

purplefox commented 11 years ago

I'm not really following this. How does runOnLoop not accomplish the same aim as nextTick (in Node.js) ?

blalor commented 11 years ago

@purplefox: runOnLoop isn't available in the global scope. If you require Q, Q doesn't know about vertex.runOnLoop without requiring vertx. It probably matters less whether it's named nextTick or runOnLoop or setImmediate, but it has to be available without the cross-engine library having specifically pull in the engine-specific library as a dependency. That's what I think @kriskowal is getting at.

kriskowal commented 11 years ago

@pidster I would greatly prefer the setImmediate standard over Node’s non-standard process.nextTick. It is not tenable for every cross-engine library to handle an unlimited number of variations of the same feature.

However, Q already does smooth over differences between a lot of engines. Adding support for Vert.X is well within its charter. I just need a way to do it that is not detrimental to those other engines (touching the org namespace, try/catch around require). If there is a global I can feature-test, that’s all I need. If there is not a global, I would recommend imlpementing setImmediate. I would not be the last to say “Thank you” for it.

purplefox commented 11 years ago

This thread has gone totally over my head. If someone wants to summarise it in non Martian, please go ahead.

purplefox commented 11 years ago

If Q is just a library that can be used from a Vert.x JavaScript verticle then the verticle will load("Q.js") and it will load ("vertx.js").

Loading vertx.js put's a global called vertx in scope, and this has a function runOnLoop. The library (Q) can then check the existence of the global.

I don't see this is significantly different to how libraries are used in node.js for example. But maybe I'm missing something.

kriskowal commented 11 years ago

@purplefox I can work with that. I can test for the existence of vertx in global scope and use vertx.runOnLoop. That’s sufficient as far as I’m concerned. It is somewhat annoying that Q will not work and that the error will appear in Q if the user does not load vertx.js first.

blalor commented 11 years ago

If you instead do require('vertx.js'), the vertx object is only in scope where the require() was called. Q and other CommonJS libraries use this best practice to isolate namespaces.

purplefox commented 11 years ago

Vert.x itself can't be loaded as a commonJS module , but that's because Vert.x is an entire platform not just a library (same as node.js in that respect).

purplefox commented 11 years ago

BTW... q looks interesting. Must look at it in more depth :)

purplefox commented 11 years ago

Hmm, it's been a long time since I looked at this, and the old memory isn't what it used to be, but, contrary to what I previously said, I think you should be able to require Vert.x as a common js module.

Take a look at the example in the distro javascript/ommonjs for more info.

Does this do what you want?

kriskowal commented 11 years ago

@purplefox Unfortunately, calling require will not help. One of the use-cases for Q is as a CommonJS module for web browsers using Browserify, Mr, or Mop. These tools treat require() calls as static dependency declarations, so require("vertex.js") would be interpreted as a dependency error for these other systems.

https://github.com/kriskowal/q/pull/150/files

purplefox commented 11 years ago

OK, so what would be your ideal solution, I'm not getting this....

kriskowal commented 11 years ago

Ideal: Vertx provides a setImmediate available in global scope (or whatever scope Vertx provides to CommonJS modules)

Acceptable compromise: Vertx provides some other global that does the same job like vertx.runOnLoop so Q would not need to call require or touch org.

Not as awesome, but I don’t care: Users have to load("vertx.js") first. Then Q checks for vertx.runOnLoop.

kriskowal commented 11 years ago

One nice thing about setImmediate is that it’s already implemented on MS IE and likely to be implemented soon on other web browsers. I’m going to go on a wild guess that Node will follow the browsers. Down that road there’s a magical future where libraries don’t have to do any shimming at all, and where library authors like myself need not even know that their library works in engines that they know very little about.

darylteo commented 11 years ago

I think (just to help Tim out with rationalising this) Would it be safe to say that the issue here is mainly of standards/convention compliance?

i.e. yes you could work around what is currently there by, say, importing the script and checking for runOnLoop, but this would be nice to have to maintain consistency and a single API to work with for all platforms in your JS script and reducing shimming.

blalor commented 11 years ago

I just filed issue #450, which somewhat confuses things. Although Vert.x does try to be a CommonJS module, its use of Rhino's load() causes the vertx object (and console) to appear in the global scope.

domenic commented 11 years ago

FYI Node has already implemented setImmediate in its master branch, i.e. the 0.9 unstable releases that will soon become the 0.10 stable release.

purplefox commented 11 years ago

Not as awesome, but I don’t care: Users have to load("vertx.js") first. Then Q checks for vertx.runOnLoop.

All vert.x JavaScript verticles (whether they use Q or not) have to first call load("vertx.js") to load the vertx API into a global (or use require() if the Vert.x commonjs support works properly), so this is not something specific to Q.

I don't think I understand yet why "load" is a bad thing. Note that Q doesn't have to call load (or require), it's the JavaScript app that uses Q that does.

I'm still confused as to what the issue is here.

darylteo commented 11 years ago

@purplefox:

As I understand it, the issue here is that in order for Q to work with vertx currently, it'd have to check and call vertx.runOnLoop.

However, there is no guarantee that Q is being used with vertx, and such a check would be additional bloat on his framework that he'd rather not add on to.

The implementation of "setImmediate" means that Q would be able to work with Vertx with no additional action on his part, and would be consistent with other platforms in a standards compliant way.

In other words, this isn't so much an issue as it is a suggestion to improve the lives of library authors.

Daryl

unscriptable commented 11 years ago

Hrrrmmmm.... "[setImmediate] is not expected to become standard, and is only implemented by recent builds of Internet Explorer. It meets resistance both from Gecko (Firefox) and Webkit (Google/Apple)" https://developer.mozilla.org/en-US/docs/DOM/window.setImmediate

tim commented 11 years ago

@darylteo https://github.com/blog/821

purplefox commented 11 years ago

Daryl,

I understand that using setImmediate would mean that Q could work without changes in different engines, but the part I didn't get was why Q would have to require in a try..catch, I can't see why that would be necessary with Vert.x, since it's the user's code that loads Vert.x (not Q), and this results in a "vertx" global added, which can be easily tested for. Perhaps the "require in a try...catch" is a red herring?

Also, slightly off-topic, I'm not sure why a promises library like Q would need to runOnLoop/setImmediate/nextTick in the first place. Calling nextTick seems to me to be the responsibility of the user of the library, not the library itself.

kriskowal commented 11 years ago

@purplefox I closed the issue 18 hours ago because doing the typeof check for vertx in global scope is sufficient for my needs. File setImmediate as “would be nice” and a different issue.

blalor commented 11 years ago

On Dec 11, 2012, at 12:49 PM, Kris Kowal notifications@github.com wrote:

@purplefox I closed the issue 18 hours ago because doing the typeof check for vertx in global scope is sufficient for my needs. File setImmediate as “would be nice” and a different issue.

I really think that's a side-effect of a bug in the Vert.x JavaScript support. It's adding objects to the global namespace even when the vert.x support is loaded with require(). Under the covers load() is used (a Rhino-specific mechanism for loading scripts into the global scope, equivalent to an eval, or multiple Githubissues.

  • Githubissues is a development platform for aggregating issues.