Exclude vertx module from browser bundles

[conradz]

TL;DR: this fixes using this module in webpack.

I ran into an error while trying to use this module with webpack. Apparently browserify does not recognize the special require that is used to test for vertx support, so it does not attempt to include it in the bundle. Webpack, however, does recognize that syntax and tries to find the vertx module, which of course fails. The proper way to exclude a module from being included when targeting the browser is to specify false for the module in the browser field in the package.json file.

[briancavalier]

Very interesting, @conradz, thanks. Yeah, we do that sneaky require <-> cjsRequest naming shuffle to avoid AMD loaders that scan module text for require.

This looks like a nice addition and no-brainer to merge, but I'll have a closer look later today.

[unscriptable]

This will probably squelch the warning message we see in rave, too.

[unscriptable]

Hmmm... actually, rave will supply a stub module for vertx if specified this way in the "browser" property. Since it will fail to throw, when.js will infer it's running under vertx. Will have to think about this a bit more.

[briancavalier]

Good point, @unscriptable. We may need to revisit this vertx sniff to see if there's a way to sniff that is friendly to webpack and all the envs we currently support.

[conradz]

Would it work to check that the returned vertx module is correct (e.g. typeof vertx.setTimer === 'function'?

[briancavalier]

@conradz That sounds like it could work. We'd still need to keep the try/catch, but maybe then additionally do the typeof check like you suggested. @unscriptable, does that seem like it would work with the stub module stuff in rave?

[unscriptable]

yep. seems like it'd work great.

[briancavalier]

Cool, let's try a test early next week.

[conradz]

OK, I updated this PR to include checking the vertx module to ensure that it is valid.

[briancavalier]

Cool, thanks @conradz. I'm about to try this out in rave and vertx. If all goes well, I'll merge it and do a patch release.

[briancavalier]

Seems to work great in rave and vertx. I found a couple of interesting things while testing, though:

1. The latest RaveJS no longer warns about vertx even without this PR. Nice :)
2. There is another spot where we sniff for vertx. I had to update that one as well. I replaced those 4 lines with the following (I'd appreciate a sanity check on this, too, to make sure I got it right :) )

            // vert.x 1.x || 2.x
            var vertx;
            try {
                vertx = cjsRequire('vertx');
            } catch (ignore) {}

            if(vertx) {
                if (typeof vertx.runOnLoop === 'function') {
                    return vertx.runOnLoop;
                }
                if(typeof vertx.runOnContext === 'function') {
                    return vertx.runOnContext;
                }
            }

@conradz How about including that in your PR as well. Also, there are minor whitespace issues. If you can update the PR for those two things, I think we'll be ready to merge!

[conradz]

OK, should be fixed. I can rebase if you want.

[briancavalier]

Looks good. I'll bump the version and get the changelog ready. Keep an eye out for a release soon. Webpack compat will be a nice addition.

Thanks again for working on this!

[conradz]

Yay, thanks for all the help you gave!

[conradz]

Just a note, this comment says that the verification of the vertx module is for webpack compat. I think this comment is wrong since the verification was added to detect the stubbed module for RaveJS, not for webpack. Webpack should still throw an error on the require, it just won't warn about the missing module when compiling with this change.

[briancavalier]

Hmm, yes, you're right.  Thanks, I'll revise the comment.

On Mon, Sep 15, 2014 at 9:18 PM, Conrad Zimmerman notifications@github.com wrote:

Just a note, this comment says that the verification of the vertx module is for webpack compat. I think this comment is wrong since the verification was added to detect the stubbed module for RaveJS, not for webpack. Webpack should still throw an error on the require, it just won't warn about the missing module when compiling with this change.

Reply to this email directly or view it on GitHub: https://github.com/cujojs/when/pull/366#issuecomment-55682895

[briancavalier]

@conradz I just fixed the comment and added more detail.