ryanve / response.js

Responsive design toolkit
http://responsejs.com
Other
801 stars 123 forks source link

Added the option to load as an AMD module. (tested with require.js) #9

Closed nstadigs closed 12 years ago

nstadigs commented 12 years ago

Note that the module requires jquery, so it wont work with zepto or jeesh when loaded as an AMD Module, but it will otherwise.

nstadigs commented 12 years ago

I know its a big diff, but I had to wrap everything in a function to be able to pass jquery as a module.

ryanve commented 12 years ago

@nbostrom Cool—I was planning on adding AMD loader support in the next version anyway. It definitely makes sense to add it. Should we include the name paramater like define('response', ['jquery'], factory) and/or should we still expose the global but add a noConflict method that lets people destroy it? jQuery does that. Also FYI read this.

nstadigs commented 12 years ago

Interesting. So the idea is to load it as an AMD module, to make sure that jQuery has loaded, but still expose it as a global? I have never thought of that solution, though I can't think of a usecase right now where it would be usefull, but you probably know better in that case. Very interesting pull request you linked to. That is indeed a better solution.

nstadigs commented 12 years ago

The problem with adding the name parameter is that it forces the user to place the script in the root folder for it to be found by the loader. If you don't specify it people are free to put it in whatever subfolder they want, eg 'lib/response.js'.

ryanve commented 12 years ago

@nbostrom I agree, anonymous seems best, esp. after reading this and this.

nstadigs commented 12 years ago

After reading around a bit I kinda regret my pull request. I think the shim method is much better, and I'll be using that method in the future. That also allows people to use zepto and jeesh if they want. I think it might be good to describe this under a "Using with an AMD loader" section in the docs though, and even describe the reason. I'll leave it to you to close this pull request if you want ;)

nstadigs commented 12 years ago

Never mind. I'll close it :P

ryanve commented 12 years ago

@nbostrom I thought about it some more too and I agree it's safest to keep the AMD logic outside of the library itself. I'll still add a noConflict method in the next version a readme section soon.

We could also put a separate file like response.min.amd.js in the repo that has the minified version ready for AMD deployment with ['jquery'] as the default, with the AMD part at the top so it's easy to customize—something like:

(function(factory) {
    define(['jquery'], factory);
}(factory)); // <- entire minified factory that takes $ as arg would be here
nstadigs commented 12 years ago

Yeah. This is probably the best way to go about it. BTW: Keep up the good work. :)

ryanve commented 12 years ago

Definitely man / thanks =]