Vert.x error with RequireJS using enforceDefine: true

[treasonx]

I ran into an error with WhenJS when trying to add better error reporting to my application.

When I enabled enforceDefine in the configuration I got the following error in my handler.

http://cloud.trx.tips/Xxg1

Basically it says:

Module name "vertx" has not been loaded yet for context: _

To workaround the problem for now I added a vertx.js file to my application dependencies with an empty define. This stops the error from being reported and the applications appears to work without any problems.

I understand there might be some issues with detecting the Vert.x environment in WhenJS which is causing the problem. I am logging the issue to help future users who may run into the same issue.

Reference: https://github.com/cujojs/when/pull/366

[unscriptable]

Hey, @treasonx! Thanks for recording this issue.

Interesting, I didn't know that RequireJS has a stricter "mode" that detects more errors.

It seems the vert.x sniffing has bitten us a few times, now. Even though you have proposed a decent work-around when using RequireJS's enforecDefine: true, I'm going to leave this open for discussion.

Just wondering if it makes sense to ask vert.x users to install a setTimeout shim rather than try to fix vert.x environments from within when.js...

Thoughts, anyone?

[briancavalier]

@treasonx Ugh, does the warning from RequireJS cause loading to halt/fail? Or does everything still work, and it's just annoying to see the warning in the console? Is there a configuration option in RequireJS to tell it to ignore a module named vertx?

It's a real pain that there's no other way to detect vert.x. I'm not sure if vert.x allows shimming the global environment. If it does, then requiring a shim would be a breaking change, so it'd probably require a 4.0 release. It's probably not just setTimeout that'd need to be shimmed, though, since we also sniff for a fast scheduling option in vertx here.

I'm really not sure what the right solution is. It's probably worth another quick investigation to see if there's any other way to detect vert.x, so I guess I'll try that route.

[treasonx]

@briancavalier I added an error handler to requirejs.onError and when I enforceDefine: true my error handler is triggered. We had some silent errors in our application due to invalid shim config for non-amd scripts. This error handler will fire if there is a mistake in your config and allows us to catch errors early in the application's life cycle. The empty define in vertx.js silences the error for now.

[briancavalier]

@treasonx I did some poking around to see if there are other ways to detect vertx, and there do appear to be some ... but they are sneaky and potentially fragile. I just pushed a change to the alternate-vertx-sniff branch that uses one of the sneaky approaches (see the very end of the last line here) first to avoid calling require (aka cjsRequire in this case) in non-vertx envs. Could you give that branch a try to see if it silences the error?

@unscriptable The sneaky approach likely only works in vert.x 2.x, which is fine with me. 1.x is pretty old now.

[stefanpenner]

neat at this._jvertx didn't realize that was a thing.

I did: https://github.com/tildeio/rsvp.js/blob/master/lib/rsvp/asap.js#L97-L98 so basically minimize the platforms where we try to require vertx.

[unscriptable]

I like that Stephan's doesn't call require unless the other alternatives are tried.

[briancavalier]

@stefanpenner thanks for the pointer, that def gave me some ideas. It seems like a key piece of info is that vertx is the only environment we support which doesn't have setTimeout. So, we can check for setTimeout first, and then only sniff for vertx if setTimeout is undefined.

Here's a gist with revisions of both async.js and timer.js that uses the "check for setTimeout first` approach. Can you guys take a look to see if it seems good?

[stefanpenner]

:+1: seems good

[briancavalier]

Thanks @stefanpenner

At some point, we should probably combine async.js and timer.js into an env.js or platform.js module so we can do these sniffs in one spot.

[briancavalier]

@treasonx The just-merged #381 should fix this issue. Can you try it out? If all is good, I'll try to release it asap. If all is not good, we can reopen this issue. Thanks!

[treasonx]

I'll give it a try in the morning and report back!

[briancavalier]

Cool, thanks.

[treasonx]

:+1: works on my machine!

[briancavalier]

@treasonx Great, thanks for confirming!

I just did a bit more reorg in #382 to clean things up, but the check is basically still the same: check for setTimeout first, then vertx. I'll try to get a 3.5.1 release out today.