optimizer forgets about main js when using wire

[boulter]

I seem to have encountered a situation when optimizing a single javascript file where the r.js optimizer will not include the main file in the optimized file when one of the required modules uses wire.js

For example, this works:

require([
  'module',
], 
function(mod) {
   // main stuff here

}, function(e) { throw e; })

this does not:

require([
  'wire!module',
], 
function(mod) {
   // main stuff here

}, function(e) { throw e; })

I uploaded a full example at http://ge.tt/91hU86E/v/0?c .

Just run optimize.sh to see the working and non-working example.

I'm using 1.0.6:

> r.js -v
r.js: 1.0.6, RequireJS: 1.0.6

[jrburke]

One minor nit: 'module' is a special, reserved module ID, more info here:

https://github.com/jrburke/requirejs/wiki/Differences-between-the-simplified-CommonJS-wrapper-and-standard-AMD-define#wiki-magic

But even when I change this to be 'mod' and mod.js, I still see the same issue. I believe it is because wire does its work in some async way, at least it does not finish when expected by r.js -- it expects the plugin to call the load callback before the plugin's load function finishes executing.

At least that is my guess, I am not too familiar with wire and when.js. Specifically I think it is when.js's chain() method that causes the issue.

I'm going to cc @briancavalier, see if he can provide some insight.

[briancavalier]

Hi guys. I'll take a look at the example you uploaded, @boulter. I can tell you, tho that wire's AMD builder plugin currently only works with cram, because it uses an API ... since the builder spec hasn't been fully solidified yet. I don't really know if that's the issue here, but hopefully we can figure it out.

I'll have some time to look at the example tomorrow, and I'll post more info then.

[briancavalier]

Actually, quick question for @boulter: Does your example load using wire when you don't optimize it? If so, then that's probably the smoking gun. If it also fails when loading unoptimized, then it sure sounds like a wire bug--although, it's been pretty rock solid with RequireJS in my experience.

[boulter]

Yeah, it works when I don't use the optimizer. This is a very simplified version of my app, of course. It took a long time to narrow it down to wire as the culprit! Thanks for looking into it.

[jrburke]

@briancavalier at first glance, the amdLoad function in wire.js looks like it would be compatible -- callback will be a function, not a promise though in r.js

I think the disconnect may be in r.js assuming that amdLoad will call the callback() before amdLoad finishes doing work (or right after, in the same sort of execution loop). My concern was that chain() is waiting to do the resolution until later. In that scenario, I could see there may be a problem.

In amdLoad, config.isBuild will be true if running in r.js, not sure if that helps make a decision on when to call callback. If the 'wire!mod' call does not result in any module loading, then something like if (config.isBuild) { callback(); return; } might be enough.

[briancavalier]

No problem. Sorry for the headache. I created an issue to add a note to the wire readme that makes it clear that it only works with cram right now.

[briancavalier]

@jrburke Interesting, thanks for the info. I'll try to explain a bit what is going on when wire.load() is called, and maybe we can figure out the best thing to do in the short and longer term.

While when.chain() doesn't do anything specifically to delay calling the callback, it's possible that something else that is happening during the wiring process is asynchronous and so could delay it. For example, something out of wire's direct control, such as one of the modules being wired, or even a wire plugin might be doing something async--wire just coordinates all those things so that they all complete before invoking the AMD load callback.

A bit more explanation on what wire!mod does: It assumes that 'mod' is the module id of a wire spec, loads it, and starts wiring it. It's goal is to call the AMD load callback once wiring has completed--again, which may happen after wire.load(). So, it seems like adding that config.isBuild check might at least cause it to fail fast, which might be a good thing in the short term.

I have to admit, I'm not really up on how r.js works. What does a truthy config.isBuild mean? In my limited understanding, it's hard for me to understand why r.js would be calling wire.load(). Is it doing that during a build? Or is this after a build and while trying to load the built file? Feel free to point me at the docs :)

Coming from wire's highly async world, it seems like a pretty tight constraint that r.js assumes that the load callback will happen synchronously. What's the reason for that?

Besides putting a note in wire's README, and potentially adding the config.isBuild fail-fast, is there anything else we could do to prevent headaches in the short term?

[briancavalier]

@boulter one other thing you might try in the short term is using wire as a module/function rather than a plugin. I'd def be curious to hear whether that works with r.js. It'd be something like this:

require([
  'wire', 'mod',
], 
function(wire, mod) {
    // Because wire may run async, it always returns
    // a promise.  Use .then() to handle the result
    wire(mod).then(
        function(wiredMod) {
            // main stuff here
        },
        function(error) {
            console.error(error);
            throw error;
        }
    );
}, function(e) { throw e; });

[jrburke]

@briancavalier:

Coming from wire's highly async world, it seems like a pretty tight constraint that r.js assumes that the load callback will happen synchronously. What's the reason for that?

For items that can be inlined in a build, they normally should be available synchronously for inclusion in a build. text templates via a text! plugin for example. It also makes building a builder much simpler, and if any define/loader plugin support were to make it into Node, I expect it would only allow loader plugins that could complete their work synchronously, to match their current module model.

To your other question, yes r.js runs loader plugins during a build. This allows them a chance to inline content into a built layer (see usual example of text templates).

For plugins that have things that normally load async (think an image or css inclusion via a link tag), then it is expected that the loader plugin would not want to include anything in the built file. For those cases testing for config.isBuild and calling callback() with no value is the way to go.

Besides putting a note in wire's README, and potentially adding the config.isBuild fail-fast, is there anything else we could do to prevent headaches in the short term?

I'm hopeful we can add the config.isBuild() check, then there won't be need for a README note about its support. :) If it helps, I can do a pull request against wire for this. It would look like so:

    function amdLoad(name, require, callback, config) {
        var resolver = callback.resolve
            ? callback
            : {
                resolve: callback,
                reject: function (err) { throw err; }
            };

        //Extra config existence check since I'm not sure
        //if curl always passes a value.
        if (config && config.isBuild) {
            resolver.resolve();
            return;
        }

        // If it's a string, try to split on ',' since it could be a comma-separated
        // list of spec module ids
        chain(wire(name.split(',')), resolver);
    }

However, this should only be done if the 'wire!mod' processing would never result in a module load where that module should be included in a build layer. I'm guessing though that since you have probably used this plugin in a build scenario with cram, this is probably true, or at least it is OK if anything it would load is not included in the build layer.

[briancavalier]

Ah, I think I understand now. I was under the impression there was a separate build-time API that would be called for a plugin, but now I get that it uses the same load() method, but passes a config where config.isBuild == true.

I think we can go one step further in a short term implementation. Ultimately, wire needs the spec modules to be in the built file, so I'm gonna plan on doing something like (or possibly exactly) the following:

if (config && config.isBuild) {
    require(name.split(','), function() { callback(); });
    return;
}

Longer term, that'll need to be expanded, but I think the algorithm that wire uses for a cram build will transfer, at least conceptually, to an r.js build.

[jrburke]

@briancavalier interesting. does wire work in a cram build, as in, does it include those modules in a build? If so, I would be curious to a pointer on how cram does this, if you know. I just assumed the load: method needs to run in the builder for things to get inlined correctly, but curious to know if there is another way to do it.

[briancavalier]

@jrburke Yeah, cram v0.2 calls a different API than load, named analyze. After getting a little more familiar with how r.js works, it seems like cram's analyze and r.js's load so very similar things. One thing that cram does is provide an API to the build plugin that abstracts platform IO.

@pieter-vanderwerff contacted me a couple days ago to let me know that he created an r.js version of wire's cram builder plugin, and said he's been using it successfully in a fairly complex project! Talk about timing. He gisted it here, so please feel free to try it out. Hopefully it will work for what you need in the short term, @boulter.

It's likely his plugin will be the basis for an r.js builder plugin in the next wire release within the next couple weeks. So, thanks to Pieter, we could have fairly complete wire+r.js build support very soon.

[boulter]

@briancavalier's suggestion of using wire.js as a module instead of a plugin worked, so I have a decent workaound. If there are additional benefits to using cram, I could switch to that.

[jrburke]

OK, so sounds like we identified how to go about solving it, it is more of a builder API adaption issue than a core bug in the r.js builder, so I'm going to close this issue now. We can reopen if there is work that r.js should do.

[briancavalier]

@jrburke Cool, sounds good. Thanks for talking through this one, guys.