-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Optionally call AMD define() to register module #338
Conversation
I would like to see this pulled in as well. AMD is more about namespace protection than asynchronous loading. |
If Underscore is already being exported globally in the same conditional body ... then what use case will this patch enable, where Underscore isn't already usable? |
To shine some light on why this would be really cool, here is how I currently use underscore in my AMD-based projects: myfile.js: require(['underscore'], function() {
// Do stuff using the global "_" reference, hoping no other part of the system has defined "_" globally
}); But with this pull request, AMD loaders can use the "define" factory method to create an underscore instance that isn't globally available: myfile.js: require(['underscore-with-amd'], function(_) {
// Do stuff using the function-local "_" reference
}); You can of course, like you say, inject underscore into the same conditional body using scripts, but then you'd need to include the same source in multiple places if you use underscore in different namespaces. You would also need a custom build system to assemble your application (vs. requirejs that just reads your dependency tree and can both load files individually in the browser, making debugging super-simple, as well as create concatenated single-file apps for production). |
…is to not leak out globals in that case.
In reply to the question from @jashkenas about use cases: The original patch in this pull request was to minimize the impact on behavior for underscore, but the main use case is to allow not bleeding globals and being able to build contained libraries that use underscore that have their own version of underscore that does not interfere with the global on the page. This is possible with AMD because the requirejs optimizer can be used to rename the AMD calls to namespace-specific items, or if a single file library is being generated and even the AMD API should not be globally available since there will be no dynamic loading, the almond AMD API shim can be used to completely encapsulate the library without using an AMD loader. Using _.noConflict does not give 100% coverage in this case -- if the library dynamically loads underscore, in IE, the script onload callback (the first opportunity for the library to call noConflict) can actually fire after other dynamic scripts load, and if one of those dynamically loaded scripts wants a different version of underscore, they could bind to the wrong version, since IE can execute a few scripts before calling the first script's onload handler. Furthermore, by using string names for dependencies as AMD does, it allows swapping out implementations without the sub-library having to export a specific global name. This is really useful for creating mocks for testing, and in the case of libraries that use jQuery, they can swap in a lighter library that exports jQuery's API/behavior without having to code for a bunch of specific global names. The original patch I put up required the library developer that wants this case to manually call _.noConflict() in their code if they want these use cases. Not the ideal for the library developer, and it had the noConflict edge case, but the patch caused the minimal amount of behavior change for underscore. In other libraries, having a global particularly for their unit tests was important. However for underscore, since a new test page that uses an AMD loader is not being included, this is not an issue. I have tested it in an AMD project, and I am willing to own any issues reported to underscore with this registration. It is actually best if underscore does not export a global if define() is available. Therefore, I have updated this pull request to not export a global if define() is available. However, if you are uncomfortable with the latest version of the pull request, I can revert it back to the original one. |
Optionally call AMD define() to register module
Thanks for the fix -- putting it in its own conditional makes much more sense. |
Should
because the location of the file doesn't match the moduleId. I use alias in require configuration right now. Am I missing something? instead be
Having that "underscore" there means that I can't do stuff like
|
@KidkArolis: I just created a document that explains some of the choices when upgrading an existing library for AMD, and it has a section on named vs. anonymous modules that explains why underscore does a named module call. Feel free to contact the amd-implement list if you want to talk more about the philosophy behind the choice. |
Awesome. Thanks! |
Read the "Unserscore is so important we will use named define(" monologue at https://github.com/jrburke/requirejs/wiki/Updating-existing-libraries Still did not see a good-looking example of loading underscore from a file-name that is not "[root]/underscore[.js]" Is there a good example somewhere on how to require underscore that is actually located in a file "js/libs/underscore-1.2.3.min.js" and getting a ref for it in require( callback? do I have to double-require it?
frankly, i would rather see you go back to anonymous define in unserscore and say to users:
You get the preloading bonus and it works as expected. Having file named with version and all is rather simple cache-busting approach. Would hate to have underscore require it to be named underscore.js and hosted at root. |
@dvdotsenko all AMD loaders allow mapping a module ID to a partial path, usually the configuration is called 'paths', so to do what you want: requirejs.config({
paths:
underscore: 'js/libs/underscore-1.2.3.min'
}
});
require(['underscore'], function () {}); Since underscore is used by other higher-level modules, like backbone, a common dependency name needs to be used to communicate a common dependency on underscore, and it makes sense to call that dependency 'underscore'. The paths config gives a way to do the mapping to a specific URL you want to use for that dependency. |
Sorry to bother. Never mind the comment above. Switched to curl.js and underscore.js just started working with custom paths in config. All good on underscore.js front. |
Yes, indeed. path helps. My, albeit subjective, two issues were:
I admit, my use-cases are not mainstream, but was seriously disenchanted with how brittle RequireJS was with modules that define themselved named inside the module. I am happy to inline my own named define(, but, obviously, with hardcoded names in modules that is not going anywhere unless i change the module (not a proposition for CDN-hosted common modules). |
@dvdotsenko If a config value is not applied until after the target resource has been initially mentioned I can see there being problems. For loading multiple versions of jQuery in a page, requirejs has multiversion support. Although it sounds like you wanted different versions for timeout recovery, so you may be interested in this recent thread on timeout recovery. If you are interested in talking through it or working out the use case a bit more, please feel free to start a thread on the requirejs list since we are getting outside the concerns of underscore. |
Indeed conversation digressed towards peculiarities of RequireJS, but, the issue of hard-coding a name INTO an AMD module is still nagging me in respect to Unserscore. Where can read up on definitive explanation for why Unserscore MUST be named AMD module, so I would stop nagging you? |
@dvdotsenko The updating existing libraries page, anon section attempts to give the reasoning for the named module guidance for libraries like jQuery and underscore. If you would prefer to discuss the relative merits of this guidance, it would be great if you can start a thread on the amd-implement list. This guidance is not specific to underscore, and we will have the opportunity to engage people who are very motivated to get this right in general for AMD. AMD loader implementers, like John Hann who develops the curl loader, is also on that list. We can post back any summary that changes the guidance to this bug, or better yet if it results in a possible code change, just follow it up with a targeted pull request citing the new common understanding worked out in the other group. |
This issue has come up before, in Issue #287. This pull request is different in these two aspects:
Patch
It is done as part of the "set global" branch, and it does not interfere with that branch, just optionally calls define if it is available. So this should not cause issues with code that still wants to use underscore as a global even when an AMD loader is on the page.
Context
In the previous pull/issue there were concerns that a global define not being part of too many useful JS runtimes.
define() is part of the AMD API which is being adopted by a few toolkits like Dojo, MooTools and EmbedJS. jQuery 1.7 will include a very similar opt-in define() call as specified in this patch. define()-based code can run in Node via the requirejs package, and define() can be used in jetpack/add-on SDK add-ons for Firefox. Firebug 1.8+ uses define().
More reasoning and context for AMD.