gvas / knockout-jqueryui

Knockout bindings for the jQuery UI widgets.
http://gvas.github.com/knockout-jqueryui/
MIT License
103 stars 38 forks source link

Support for RequireJS/AMD Modules #2

Closed juuxstar closed 11 years ago

juuxstar commented 11 years ago

Nice job on this integration library especially the abstraction of the bindingFactory. Makes adding and maintaining the widgets really simple. It's the main reason what swung me to committing to incorporating your library into our production code base.

Speaking of which, I've recently started a new project with Knockout and JQueryUI widgets so your library came in really handy and I'm very happy with how it's currently going. I'm also using the RequireJS library for dependency management so I refactored your code to work as AMD-style modules which has the nice benefit of not creating any extra global variables and not depending on global variables (ie. JQuery, Knockout, and JQuery-UI). Instead, these are requires that must be satisfied by configuring requireJS in the encompassing project. (though is a very simple step)

Anyway, what I was wondering if you are interested in incorporating this refactoring into your main repo so that others can also make use of it. It would take a bit of work to figure out how to make the library use requireJS when present and use globals as a fallback (I have seen this type of pattern with other libraries, for example: Knockout itself uses it, see: http://knockoutjs.com/documentation/amd-loading.html)

Let me know what you think.

Cheers,

Tomas

semmel commented 11 years ago

Thumbs up for this feature! I use RequireJS everywhere I can but sadly I often have to wrap 3rd party libraries to make them AMD-style compatible. But I hate to integrate a modified version of other peoples code.

Since Knockout itself is advocating module loading via RequireJS, please integrate the additions from juuxstar!

Until then as a quick fix I use the following shim in my initRequireJs.js file:

requirejs.config({
    paths: {
        jquery: ['vendor/jquery'],
        jquery_ui: ['vendor/jquery-ui.custom'],
        knockout: ['vendor/knockout'],
        knockout_jquery_ui: ['vendor/knockout-jquery-ui']
    },
    shim: {
        jquery_ui: {
            deps: ['jquery'],
            exports: '$.ui'},
        knockout_jquery_ui: {
            deps: ['jquery', 'jquery_ui', 'knockout'],
            exports: 'kojqui'
        }
    }
});

But you have to reintroduce the global ko variable

require(['knockout'], function(ko)
{
    window.ko = ko;
});

before loading knockout-jqueryUI.

With best wishes Semmel

gvas commented 11 years ago

@juuxstar: Sorry for the late response. I'd be more than happy to add support for AMD. Can you incorporate your changes into a pull request? Or should we discuss the necessary refactoring beforehand?

Regards, Gábor

gvas commented 11 years ago

I've created a new branch named 'modular'. I'll merge it into master in the next couple of days.

juuxstar commented 11 years ago

Yeah some planning is probably warranted. I simply refactored you code to just use RequireJS modules so it integrates nicely with a project that also uses RequireJS but in the current state does not work without require.

RequireJS also comes with it's own optimizer tool which stitches multiple modules together (and minifies them) but just concatenating module files (as per your grunt script) does not work since require expects only one module per file.

Ideal Situation:

I think I know how to get this accomplished by adding a function wrapper around the whole definition, setting up the dependent libraries (KO, JQuery) as locals (either via require or globals if require doesn't exist) so that each widget definition can use them as "globals", and also an export function that switches export behaviour depending on what's available in the environment (RequireJS vs globals). The last part I'll probably borrow from how KO itself did it.

gvas commented 11 years ago

Thanks, I've already wrapped the library with the factory function from umd, and modified the source files, the specs and the build script. You should check the modular branch. Actually I think it is already ready to release, but I am yet to see how well does it play in a RequireJS environment.

juuxstar commented 11 years ago

Very nice! Works great out of the box! It even cleans up some of the utils that dealt with managing the namespaces.

For requireJS projects, here's what's then needed in the config setup to satisfy the dependencies:

require.config({
    paths : {
        "jquery"   : "some/path/to/jquery-X.Y.Z",
        "jquery-ui" : "some/path/to/jquery-ui-X.Y.Z",
        "knockout" : "some/path/to/knockout-X.Y.Z"
    }
    shim: {
        'knockout' : [ 'jquery' ],
        'jquery-ui' : [ 'jquery' ],
    }
});

This could be added to the documentation to make it easier for RequireJS users to integrate the library.

Excellent job again! The umd project was a great find. I'll have to remember it for any future libraries.

gvas commented 11 years ago

Thanks. I'll update the documentation with your config example. Minor correction: knockout doesn't depend on jquery, so the first line in the shim config is unnecessary.

juuxstar commented 11 years ago

You sure? Looking at the Knockout 2.2.1 source, it looks like jQuery is being referenced in a number of places but looks like there are fallback methods each time so it's probably optional. So strictly speaking, you are correct.

However, in at least one instance, ko.utils.setHtml, using jQuery's method is better than the fallback logic. (I already ran into this when using the html binding to set dynamic content) Thus I would recommend giving KO the JQuery dependency especially if the project is already using it anyway.

gvas commented 11 years ago

True.

I've documented the module support: http://gvas.github.io/knockout-jqueryui/install.html

@juuxstar, @semmel: thanks for your ideas/suggestions!