Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from
Closed

Conversation

nstadigs
Copy link

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

Note that the module requires jquery, so it wont work with zepto or jeesh
when loaded as an AMD Module
@nstadigs
Copy link
Author

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
Copy link
Owner

ryanve commented May 15, 2012

@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
Copy link
Author

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
Copy link
Author

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
Copy link
Owner

ryanve commented May 16, 2012

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

@nstadigs
Copy link
Author

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
Copy link
Author

Never mind. I'll close it :P

@nstadigs nstadigs closed this May 17, 2012
@ryanve
Copy link
Owner

ryanve commented May 29, 2012

@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
Copy link
Author

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

@ryanve
Copy link
Owner

ryanve commented Jun 1, 2012

Definitely man / thanks =]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants