Bug: UMD pattern treated like RequireJS even when not present

[Munter]

When running buildProduction --root backbone --outroot backbone-dist backbone/index.html in https://github.com/Munter/todomvc-challenge/tree/gh-pages/architecture-examples/backbone I get this log:

munter at pollex in ~/assetgraph/todomvc-challenge/architecture-examples on gh-pages
$ buildProduction --root backbone --outroot backbone-dist backbone/index.html
 ✔ 0.000 secs: registerRequireJsConfig
 ✔ 0.002 secs: registerLabelsAsCustomProtocols
 ✔ 0.111 secs: loadAssets
 ✔ 0.439 secs: populate
 ✔ 0.265 secs: assumeRequireJsConfigHasBeenFound
 ⚠ file:///home/munter/assetgraph/todomvc-challenge/architecture-examples/backbone/underscore.js: ENOENT, open '/home/munter/assetgraph/todomvc-challenge/architecture-examples/backbone/underscore.js'
Including assets:
    file:///home/munter/assetgraph/todomvc-challenge/architecture-examples/backbone/bower_components/backbone.localStorage/backbone.localStorage.js

 ⚠ file:///home/munter/assetgraph/todomvc-challenge/architecture-examples/backbone/backbone.js: ENOENT, open '/home/munter/assetgraph/todomvc-challenge/architecture-examples/backbone/backbone.js'
Including assets:
    file:///home/munter/assetgraph/todomvc-challenge/architecture-examples/backbone/bower_components/backbone.localStorage/backbone.localStorage.js

 ✔ 0.074 secs: populate
 ✔ 0.000 secs: fixBaseAssetsOfUnresolvedOutgoingRelationsFromHtmlFragments
 ✔ 0.000 secs: populate
 ✔ 0.001 secs: removeRelations
 ✔ 0.028 secs: addDataVersionAttributeToHtmlElement
 ✔ 0.004 secs: compileLessToCss
 ✔ 0.001 secs: removeRelations
 ✔ 0.002 secs: populate
 ✔ 0.194 secs: stripDebug
 ✔ 0.000 secs: removeRelations
 ✔ 0.000 secs: externalizeRelations
 ✔ 0.000 secs: mergeIdenticalAssets
 ✔ 0.015 secs: processImages
 ✔ 0.001 secs: spriteBackgroundImages
 ✔ 0.020 secs: processImages
 ✔ 0.003 secs: inlineKnockoutJsTemplates
 ✔ 0.001 secs: liftUpJavaScriptRequireJsCommonJsCompatibilityRequire
 ✔ 0.043 secs: flattenRequireJs
 ✔ 0.001 secs: removeUnreferencedAssets
 ✔ 0.001 secs: convertCssImportsToHtmlStyles
 ✔ 0.000 secs: removeDuplicateHtmlStyles
 ✔ 0.003 secs: mergeIdenticalAssets
 ✔ 0.000 secs: bundleRelations
 ✔ 0.001 secs: splitCssIfIeLimitIsReached
 ✔ 0.000 secs: replaceRequireJsWithAlmond
 ✔ 0.050 secs: bundleRelations
 ✔ 0.332 secs: mergeIdenticalAssets
 ✔ 0.000 secs: removeNobundleAttribute
 ✔ 0.047 secs: inlineCssImagesWithLegacyFallback
 ✔ 0.021 secs: minifyAssets
 ✔ 0.001 secs: removeAssets
 ✔ 0.001 secs: removeAssets
 ✔ 3.279 secs: compressJavaScript
 ✔ 0.000 secs: inlineRelations
 ✔ 0.000 secs: inlineAngularJsTemplates
 ✔ 0.000 secs: setAsyncOrDeferOnHtmlScripts
 ✔ 0.000 secs: omitFunctionCall
 ✔ 0.000 secs: inlineRelations
 ✔ 0.313 secs: moveAssetsInOrder
 ✔ 6.147 secs: buildProduction
 ✔ 0.020 secs: writeAssetsToDisc
      Html 1   1.7 KB
       Css 2  17.4 KB
       Png 1   2.1 KB
JavaScript 1 129.8 KB
    Total: 5 150.9 KB
 ✔ 0.000 secs: writeStatsToStderr

Looks like Assetgraph is interpreting this section using the RequireJS branch even though requirejs isn't present: https://github.com/Munter/todomvc-challenge/blob/gh-pages/architecture-examples/backbone/bower_components/backbone.localStorage/backbone.localStorage.js#L7-L18

[Munter]

The build output also includes this:

... define("backbone.localStorage",["underscore","backbone"],function(e,t){ ...

[Munter]

This file also gives similar trouble: https://github.com/passy/canjs-localstorage/blob/master/can.localstorage.js#L2-L9

[Munter]

This problem turns out to be a bit more severe than I thought. Not only is the whole UMD pattern replaced with a call to an undefined define function, but the actual factory code is also stripped away completely.

Before:

(function (root, factory) {
    if (typeof define === 'function' && define.amd) {
        define(['backbone'], function (Backbone) {
            return factory(Backbone || root.Backbone);
        });
    } else {
        factory(Backbone);
    }
}(this, function (Backbone) {
    Backbone.LocalStorage = 'I am the local storage. Muha!';

    return Backbone.LocalStorage;
}));

After:

define("backbone.localStorage", ["backbone"], function(Backbone) {
    return factory(Backbone || root.Backbone);
});

I think we might need to drop our fanciness with trying to optimize away the UMD wrapper and just live with the overhead so the runtime can do it's thing. Obviously this thing also throws a ENOENT because Assetgraph tries to populate the define dependency even though RequireJS isn't loaded and the JS asset has no incoming RequireJS-like relations.

[Munter]

I've added some changes to the unrequired branch. It is breaking a million test cases at the moment, but it does build the above example correctly it seems.

My strategy is to add an isRequired getter to the JavaScript asset that tells me if the asset has any incoming relations of types that indicate if it is included through RequireJS or AMD. I'm using this as a contition to not populate outgoing RequireJS/AMD relations, and to skip some of the UMD rewriting magic we've done in the flattenRequireJS transform.

Am I on the right track here?

[Munter]

My method of using a getter that looked at the assets incoming relations turned out not to be working properly in cases where the asst would be cloned, like in the flattenRequireJs transform. I instead switched my approach to registering an isRequired boolean at the time when any requirejs type relation is instantiated. This boolean is persisted when cloning. Pull request coming up