-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Conversation
Thanks for your contribution, Karel! :) |
Indeed I missed quite some files :) |
AMD is cool, loving it and using it everyday but I don't think you should use it to require jquery.ui.widget and others. Those are not exactly dependencies but more "requirements", also you shouldn't ask for specific folder structure. |
What would your recommendation be? The docs on requirejs.org recommend to reference the dependencies by path names and using anonymous modules instead of named modules: If you require for example jquery.fileupload-ui.js it depends on jquery.fileupload-ip.js, which in turn depends on jquery.fileupload.js which definitely depends on the jQuery widget factory, as it doesn't work without it. If I implement support for AMD loaders, I want to implement it in a way that works by just requiring one script (jquery.fileupload-ui.js) which should load all the dependencies, including the widget factory. jQuery itself is a bit of a special case, as it's usually not loaded via AMD, even when AMD is used for other scripts. Anyway, if you have any recommendation for improvements, I'd like to hear about them. :) |
Actually, I had issues specifically with loading vendor/query.ui.widget as I don't want it to be there (got an existing folder structure I really care about). It seems like the paths configuration doesn't work with this kind of relative path. Turns out I don't know if it's a bug, a feature from require.js or just me doing something wrong, missing something from the doc. I'll ask @jrburke about this one. Here's a gist with the confs I tried : https://gist.github.com/1955536 I agree about jQuery, special case but I also saw some bugs with this one... |
Hmm, might be that scripts included via subfolders shouldn't use the ./ at the front, but it's a valid path and it worked in my tests with requirejs. |
For external dependencies, like jquery.ui.widget, those should be specified just as 'jquery.ui.widget', and only code that is considered part of this package should use relative IDs, the ones that start with './'. This allows better cross sharing of those common external dependencies. For instance, if I also have another lib that depends on jquery.ui.widget, if that lib used something like './external/jquery.ui.widget', that would result in two copies of jquery.ui.widget getting loaded. By using 'jquery.ui.widget', it means that the project that uses both libs can create a path config to the right copy to use for jquery.ui.widget, as it likely has its own copy anyway, maybe even the whole jQuery UI lib. Also, relative IDs are mapped relative to the reference ID, not their path, so if I use this paths config: {
paths: {
'jquery.fileupload': 'libs/jQuery-File-Upload/js/jquery.fileupload'
}
} Then for jquery.fileupload.js, its ID is 'jquery.fileupload', and its relative dependency './jquery.iframe-transport', is resolved relative to 'jquery.fileupload', so the final absolute ID is 'jquery.iframe-transport', which is looked up in baseUrl + 'jquery.iframe-transport' + '.js'. For that to work, either include a map for 'jquery.iframe-transport': {
paths: {
'jquery.fileupload': 'libs/jQuery-File-Upload/js/jquery.fileupload'
'jquery.iframe-transport': 'libs/jQuery-File-Upload/js/jquery.iframe-transport'
}
} or ask consumers of your library to just map the whole JS file as a "package name": {
paths: {
'fileupload': 'libs/jQuery-File-Upload/js'
}
} then consumers of the lib would ask for 'fileupload/jquery.fileupload' as the dependency name. Or, if the libraries in the JS file are really useful just as standalone libraries on their own, ask developers to just place them all in the baseUrl of the user's project. In that case, then I would not use relative IDs for dependencies for the files in the js dir. |
Thanks for the clarifications, James. :) |
I edited the files to bring AMD support to the module. If you don't know about AMD, jquery >1.7 is using it and it is growing really fast. There is a lot of lecture on the web about it.
Regarding the edits I made to your code, it consists in wrapping the files into a define() method if the app use AMD and fall back to globals otherwise. You can read about it here: https://github.com/umdjs/umd
I didn't touch anything else in your code and I did a quick test with an app using AMD and one not and everything seems to work just fine.