Fix for #938: always export ko as global

[dotnetwise]

As per #938 and many other reported bugs on 3rd party libraries i.e. https://github.com/Knockout-Contrib/Knockout-Validation/issues/259

Because some of the third party libraries such as knockout validation are not aware of AMD we need to always export ko to the global namespace. Otherwise when optimizing multiple modules with AMD including knockout itself, the other 3rd party modules would simply fail because they would always rely on the global ko variable. This pull request easily fixes that case, and is also making sure the define is not anonymous, so that the shim configuration can be used side-by-side with the default ko module.

[brigand]

It seems like this will call factory twice. If you look at this part, and assume the the if statement will evaluate to true...

if (typeof define === 'function' && define['amd']) {
    // define will invoke factory
    define('ko', ['exports'], factory);
}

// we're invoking factory
factory(window['ko'] = {});

Wouldn't this make more sense?

(function(factory) {    
    // Support three module loading scenarios
    if (typeof require === 'function' && typeof exports === 'object' && typeof module === 'object') {
        // [1] CommonJS/Node.js
        var target = module['exports'] || exports; // module.exports is for Node.js
        factory(target);
        window['ko'] = target;
    } else if (typeof define === 'function' && define['amd']) {
        // [2] AMD anonymous module
        var ko;

        define('ko', ['exports'], function(exportObject){
            ko = exportObject;
            factory.apply(this, arguments);
        });

        window['ko'] = ko;
    } 
    else {
        // [3] No module loader (plain <script> tag) - put directly in global namespace
        window['ko'] = factory({});    
    }    
}(function(koExports){

---

By the way, what's clanhub?

[gliljas]

I'd like to stress the importance of this. Including RequireJS in your project shouldn't have to mean that you surrender completely to the Require way. Sure, adding things to the global namespace is bad practice, but destroying the extensibility API is even worse. Other libraries and frameworks are usually perfectly fine with adding their extensibility points to window.XX, and I can count the number of times that has turned out to be a problem on the toes of my left hand.

At the very least, two different builds could be provided. I want to use RequireJS and Knockout together, but

1. I can handle it with a "return window.ko"
2. I don't want to have to rewrite all bindingHandlers with a convoluted factory syntax

[crissdev]

IMO, this issue should be closed. Not having ko in global namespace is really not bad. I never needed it while working with RequireJS. Other components relying on ko to be present in the global namespace should be wrapped in a define statetement - grunt, gulp are really good at this.

[gliljas]

I respectfully disagree. Not only because it turns the extensibility API into a convoluted mess, not aligned with the documentation on how to write binding handlers, components etc, but also because dynamic module loading doesn't (yet) make any sense in most scenarios. The official documentation gives an example:

http://knockoutjs.com/documentation/amd-loading.html#loading-knockoutjs-a-binding-handler-and-a-viewmodel-class-via-requirejs

..where the binding handler is specified as a dependency on the viewmodel. IMO that is very wrong and not a kind of coupling that should be promoted.

That said, I wouldn't mind if also binding handler could have a "require" setting, allowing them to be loaded (but not configured) together with possible dependencies.

[SteveSanderson]

Does anyone still have an opinion on this, or should we close it? I'm open to doing this if people still feel there's a need.