Skip to content

Commit

Permalink
Removing AMD 'support' from Underscore.
Browse files Browse the repository at this point in the history
  • Loading branch information
@jashkenas
jashkenas committed Jan 11, 2012
1 parent 0b74f2d commit 0d4b124
Showing 1 changed file with 4 additions and 9 deletions.
13 changes: 4 additions & 9 deletions underscore.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,21 +48,16 @@
// Create a safe reference to the Underscore object for use below.
var _ = function(obj) { return new wrapper(obj); };

// Export the Underscore object for **Node.js** and **"CommonJS"**, with
// backwards-compatibility for the old `require()` API. If we're not in
// CommonJS, add `_` to the global object.
// Export the Underscore object for **Node.js**, with
// backwards-compatibility for the old `require()` API. If we're in
// the browser, add `_` as a global object via a string identifier,
// for Closure Compiler "advanced" mode.
if (typeof exports !== 'undefined') {
if (typeof module !== 'undefined' && module.exports) {
exports = module.exports = _;
}
exports._ = _;
} else if (typeof define === 'function' && define.amd) {
// Register as a named module with AMD.
define('underscore', function() {
return _;
});
} else {
// Exported as a string, for Closure Compiler "advanced" mode.
root['_'] = _;

This comment has been minimized.

Sign in to view

Copy link
@jdalton

jdalton Jan 11, 2012

Contributor

Why remove that comment about Closure compiler?

This comment has been minimized.

Sign in to view

Copy link
@jashkenas

jashkenas Jan 11, 2012

Author Owner

It just moved up a couple lines, so this will look a 'lil bit nicer:

http://underscorejs.org/docs/underscore.html#section-10

This comment has been minimized.

Sign in to view

Copy link
@jdalton

jdalton Jan 11, 2012

Contributor

Ahh yap, cool.
}

Expand Down

17 comments on commit 0d4b124

@ryanflorence
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's ironic, but this actually allows AMD users to use backbone in an amd environment without hacking the source.

Backbone checks for root._ but underscore didn't export to root when define was detected, and since underscore is a dependency it was all busted.

Now you can write a little shim to get all the libs you need for backbone w/o touching the source of any of them.

define ['vendor/underscore', 'vendor/jquery', 'order!vendor/backbone'] -> Backbone

Good move.

@jashkenas
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. Not supporting a particular script loader will definitely make it easier for all of them to work properly.

I apologize for merging the support in the first place.

@ryanflorence
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For those interested it's possible to support node, amd, and the usual global module patterns with dependencies, but it takes a disappointing amount of boilerplate (https://github.com/umdjs/umd/blob/master/returnExports.js). I still think its worth it, but whatever ...

When amd becomes more standard, maybe it'll be worth it to @documentcloud. For now I think this commit is the right move though.

@iammerrick
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ryan I am a little embarrassed to say I didn't recognize your CoffeeScript as code and thought you were saying that those dependencies were required for each module to get backbone thus the arrow.

To spare anyone from the same ignorance. Here is the JS shim. :-)

// backbone.js shim

define(['vendor/underscore', 'vendor/jquery', 'order!vendor/backbone'], function(){
    return Backbone;
});

Sorry dude.

@ryanflorence
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Merrick,

yeah so:

app/
- scripts/
  - backbone.js
  - vendor/
    - backbone.js
    - jquery.js
    - underscore.js

Merrick's code being app/backbone.js, a little shim module to give you direct access to backbone as a module. Could even toss some noConflicts in there to remove everything from the global scope.

@jdalton
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dig AMD support (why not?), never understood @jashkenas' rage/push against it.

@millermedeiros
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't recommend using the order! plugin, it may fail depending on the server settings and isn't really required in this case, the depend! plugin is a better fit for this case:

define(['depend!vendor/backbone[jquery,underscore]'], function(){
    return Backbone;
});

jQuery should be listed as jquery since it registers itself as a named module...

another option is to wrap Backbone into an AMD module (since it has dependencies):

define(['jquery', 'underscore'], function($, _){
  // paste Backbone.js source code here
  return Backbone;
});

it was a mix of a "bad" and "good" move, good since it won't register it as a named module (named modules are usually a bad thing for portability) and bad since the Backbone conflict could be solved by wrapping it as an UMD...

if you are concerned that wrapping it into an AMD module might make updates of third-party libs harder automate ALL things!!

#!/bin/bash
echo -e "\n -- updating underscore.js --\n"

echo -e "define(function(){\n//start wrap---" > js/lib/underscore.js
curl https://raw.github.com/documentcloud/underscore/master/underscore.js >> js/lib/underscore.js
echo -e "//end wrap---\nvar exports = _.noConflict();\nreturn exports;\n});" >> js/lib/underscore.js

echo -e "\n -- updating backbone.js --\n"

echo -e "define(['jquery', 'underscore'], function($, _){\n//start wrap---" > js/lib/backbone.js
curl https://raw.github.com/documentcloud/backbone/master/backbone.js >> js/lib/backbone.js
echo -e "//end wrap---\nvar exports = Backbone.noConflict();\nreturn exports;\n});" >> js/lib/backbone.js

or if you prefer to use a helper lib that is already in the AMD format you can try AMD-utils, it doesn't have all the features from underscore but it has a few extras as well... (and you only load what you need)

AMD FTW

EDIT: updated depend! plugin example (since it was redundant) and also updated bash script to add -e flags.

@millermedeiros
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just saw @jashkenas twit

… because AMD support is breaking regular Underscore embeds on pages that also happen to use Require.js …

so instead of dropping the define(), why not keep the browser exports but do a unnamed define instead?

if (typeof exports !== 'undefined') {
  if (typeof module !== 'undefined' && module.exports) {
    exports = module.exports = _;
  }
  exports._ = _;
} else {
  if (typeof define === 'function' && define.amd) {
    //register as an unnamed module but still export it as a global
    //object to avoid breaking `undescore` in pages that also happen
    //to use an AMD loader
    define(function(){
      return _;
    });
  }
  root['_'] = _;
}

the user can still call _.noConflict() if needed..

@wjcrowcroft
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tend to agree with this, but on the fence about removing the similar lines that I stuck in some of my JS micro libs, since they seem to work great. Was taking underscore as a best practice at the time.. Guess it's different here because of underscore's serving as Backbone's dependency.

I'd say if removing this it'd be nice to add a note in the docs about AMD, perhaps linking to some article explaining how to set up that easy shim which would export underscore and Backbone in noConflict for when people need it.

@jrburke
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @millermedeiros: the problem can be fixed by also exporting the global in addition to the define() call. To be clear, this code change was not for supporting a particular script loader, but a class of script loaders that support modular code. It is difficult upgrading the web, and while ideally underscore would not need to export the global if AMD support is on a page, there is a bootstrap period where it helps.

I hope this code change is reconsidered, feel free to ping me if there is a way I can help.

@jdalton
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jQuery has AMD support and a huge web presence without problems.
Maybe underscore.js should look at how jQuery bootstraps its support.
(I think jQuery exports a global reference in the meantime too)

@jrburke
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jQuery exports a global in addition to calling define. I'm happy to do a pull request if that helps.

@jrburke
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and did this pull request to show how the issue could be resolved without removing define() support.

@svieira
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jashkenas - I'm in agreement with @millermedeiros and @JBurke - it would be better to have support for AMD if it can be had without breaking existing libraries. I'm using curl.js for some projects right now and when I saw that underscore had updated I was looking forward to upgrading to the newest version of underscore - only to find that the update breaks support for the loader+environment I am using it in. I'm willing to maintain a fork if that's what's necessary - but that seems like unnecessary fragmentation of a really vibrant project.

@xcambar
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although it has its imperfections of its own, the solution provided by the jQuery team is IMO the wiser. It supports the 'traditional' global export while still allowing AMD through the registration of a named module.

Developers that use AMD generally don't rely on globals at all. The only thing I would constrain is about exporting the global: that should ideally be done if and only if the define() function is not available.

@jrburke, your pull request is close to what is to me the ideal solution. To be more specific, the first commit if the pull request is the ideal solution, the second degrades it (with all due respect, obviously). But I'm curious about what had you revert to a "global always" implementation.

@jrburke
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xcambar: I'm not sure I follow your comment, but the global registration needs to be there when an amd loader loads underscore and another script that depends on underscore but does not been call define() for their dependencies. For instance, backbone/master.

If underscore only calls define in that case and does not export a global, then an error occurs. It is also useful when multiple versions of underscore could be loaded on a page (even outside an amd loader), but the loader may only want one of them. If the underscore definition disappears into define, another script that wanted to use it as a global would have a problem.

Basically the same forces are at play for underscore as they are for jQuery, so the same pattern is used. Most other libs do not have this problem though. It just happens to be that underscore and jQuery are really popular, and modular loading is really new for most developers.

If you want to talk about it more, it is probably best to do outside of this commit, to avoid the extra noise for the underscore collaborators.

@ryanflorence
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bardu https://github.com/amdjs/backbone is the drop-in AMD-compatible fork for backbone.

Please sign in to comment.