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

Optionally call AMD define() to register module #338

Merged
merged 3 commits into from Oct 19, 2011
Merged

Optionally call AMD define() to register module #338

merged 3 commits into from Oct 19, 2011

Conversation

jrburke
Copy link
Contributor

@jrburke jrburke commented Oct 18, 2011

This issue has come up before, in Issue #287. This pull request is different in these two aspects:

  • The patch itself is different, doing it optionally as part of the global registration branch.
  • I can provide more context on why this is useful to do.

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.

@taybin
Copy link

taybin commented Oct 19, 2011

I would like to see this pulled in as well. AMD is more about namespace protection than asynchronous loading.

@jashkenas
Copy link
Owner

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?

@jakewins
Copy link

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

@jrburke
Copy link
Contributor Author

jrburke commented Oct 19, 2011

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.

jashkenas added a commit that referenced this pull request Oct 19, 2011
Optionally call AMD define() to register module
@jashkenas jashkenas merged commit ebca7f2 into jashkenas:master Oct 19, 2011
@jashkenas
Copy link
Owner

Thanks for the fix -- putting it in its own conditional makes much more sense.

@KidkArolis
Copy link

Should

define('underscore', function() {
  return _;
});

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

define(function() {
  return _;
});

Having that "underscore" there means that I can't do stuff like

define(["vendors/underscore"], function (_) {});

@jrburke
Copy link
Contributor Author

jrburke commented Nov 30, 2011

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

@KidkArolis
Copy link

Awesome. Thanks!

@dvdotsenko
Copy link

@jrburke

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?

require(["js/libs/underscore-1.2.3.min.js"], function(_){
    // Whoa! WTF? underscore is null!?
    require(['underscore','myapp'], function(_, myapp){
        // finally, do usefull things with _
    })
})

frankly, i would rather see you go back to anonymous define in unserscore and say to users:

// You want consistent, named module for 'underscore'?  insert the following into your "app_loader.js"
define(
    'underscore'
    , ["js/libs/underscore-1.2.3.min.js"]
    , function(_) {return _}
)

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.

@jrburke
Copy link
Contributor Author

jrburke commented Dec 22, 2011

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

@dvdotsenko
Copy link

@jrburke

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.

@dvdotsenko
Copy link

@jrburke

Yes, indeed. path helps. My, albeit subjective, two issues were:

  1. I was trying to pass config to inline require deeper in the code (not in index.html) and, somehow, that config did not stick for require( nested deeper in that tree. Deeper calls resolved 'jquery' to a [root]/jquery[.js]. Learned my lesson - need to use global config.
  2. Another edge issue - I wanted to have 'jquery' and 'jquery_other' (for funky CDN > local failover at first, then, to play with different versions of jQuery because some of the plugins i use were born before the .args .prop split)

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

@jrburke
Copy link
Contributor Author

jrburke commented Dec 22, 2011

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

@dvdotsenko
Copy link

@jrburke

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?

@jrburke
Copy link
Contributor Author

jrburke commented Dec 22, 2011

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

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

Successfully merging this pull request may close these issues.

None yet

9 participants