Skip to content
This repository has been archived by the owner on May 25, 2023. It is now read-only.

Add optional AMD support #942

Merged
merged 2 commits into from
Jan 16, 2012
Merged

Add optional AMD support #942

merged 2 commits into from
Jan 16, 2012

Conversation

karellm
Copy link
Contributor

@karellm karellm commented Jan 15, 2012

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.

blueimp added a commit that referenced this pull request Jan 16, 2012
Add optional AMD support
@blueimp blueimp merged commit 6afe4fc into blueimp:master Jan 16, 2012
@blueimp
Copy link
Owner

blueimp commented Jan 16, 2012

Thanks for your contribution, Karel! :)
I've committed an updated version which includes a refactored version of your AMD support implementation.
I've also added AMD support to the other plugin files of the repository.

@karellm
Copy link
Contributor Author

karellm commented Jan 16, 2012

Indeed I missed quite some files :)
Thanks for the quick merge!

@ZeeAgency
Copy link

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.

@blueimp
Copy link
Owner

blueimp commented Mar 2, 2012

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:
http://requirejs.org/docs/start.html#add
http://requirejs.org/docs/whyamd.html#namedmodules

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.
The plugin also ships with an AMDified version of the widget factory.

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.
That's why it's the only script that is loaded as named module, to allow passing jQuery as $ parameter to the plugin scripts.

Anyway, if you have any recommendation for improvements, I'd like to hear about them. :)

@ZeeAgency
Copy link

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...

@blueimp
Copy link
Owner

blueimp commented Mar 2, 2012

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.
Thanks for looking into this and please let me know when you get more information.

@jrburke
Copy link

jrburke commented Mar 2, 2012

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.

@blueimp
Copy link
Owner

blueimp commented Mar 3, 2012

Thanks for the clarifications, James. :)

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

Successfully merging this pull request may close these issues.

4 participants