Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix for Ender + Backbone #3038

Closed
wants to merge 3 commits into from

5 participants

@symmetriq

When Backbone is used as an Ender module, exports always exists, so
root.ender needs to be passed to factory. (When not using Ender,
the 4th arg will be undefined, same as before.)

Note that Ender 2.x no longer automatically integrates all modules into
$. There are two ways to use Backbone with Ender 2:

  1. Assign it to its own var and use it like this:

    var Backbone = require('backbone');
    Backbone.$ = $;
    Backbone.View.extend({…});

  2. Integrate into $ by adding --integrate backbone when building
    the Ender library, then use it like this:

    $.View.extend({…});

Jason Sims Fix for Ender + Backbone
When Backbone is used as an Ender module, `exports` always exists, so
`root.ender` needs to be passed to `factory`. (When not using Ender,
the 4th arg will be undefined, same as before.)

Note that Ender 2.x no longer automatically integrates all modules into
`$`. There are two ways to use Backbone with Ender 2:

1. Assign it to its own var and use it like this:

var Backbone = require('backbone');
Backbone.View.extend({…});

2. Integrate into `$` by adding `--integrate backbone` when building
the Ender library, then use it like this:

$.View.extend({…});
e926303
@dgbeck

What about using jquery / other dom libraries with browserify? That use case is currently totally broken, as the no value is given for the $ argument of the factory function.

I believe this patch is a step in the right direction but should be updated to:

factory(root, exports, _, (root.jQuery || root.Zepto || root.ender || root.$) );

That way there is consistency with the browser global case. Also you can use browserify-shim to shim in your choice of DOM library.

@symmetriq

Good point. That makes sense to me, as least as an interim way to get Backbone to integrate with your choice of general utility (jQuery or something compatible) when it's packaged up by tools like Ender and Browserify.

In the bigger picture, I'd prefer a more sustainable, general solution. Simply checking if 'exports' exists is not a reliable way to identify the host environment, so it doesn't seem wise to build much on top of that, but this seems to be the least messy way to get Backbone to play nicely with Ender and Browserify in the meantime.

@akre54
Collaborator

This whole thing gives me the icks. Why can't we add the require statement back inside the try-catch (reverting #2997), and then force the browserify user to stub out jQuery if they're not using it? We also need a better test that won't trip up Ender here, as passing root.ender in what is supposed to be the commonjs path also feels gross.

@symmetriq

FWIW, what I'm currently doing (with Backbone as a module in my Ender-built library) is this:

(After ender[.min].js is loaded…)

var Backbone = require('backbone');
Backbone.$ = $;

This is easy enough that I haven't been all that anxious to solve the auto-integration problem.

I, too, would prefer to revert the original "exports exists, therefore…" kludge rather than adding another one on top. However, since I didn't (and still don't) really understand what problem the first change was intended to solve, I came up with this as a way of fixing the Ender integration problem while leaving the original kludge intact.

I would certainly prefer a solution that avoids both hacks, but I don't know enough about how Backbone is used in other contexts to offer one.

For my purposes I'm fine with just manually assigning Backbone.$ = $ each time.

@braddunbar
Collaborator

So this can be solved by manually setting Backbone.$? Isn't that the solution that was already decided on generally? To my knowledge, jQuery requires this as well.

@symmetriq

Manually setting Backbone.$ is a decent workaround, but if that's the intended usage, root.ender should at least be removed from the browser global factory() call (line 25), since it's unreachable when Backbone is an Ender module. (Remember, exports exists in that context, so we end up in the 2nd block, where nothing gets passed for the 4th argument.)

It seems to me like there's a real solution to be found here though, since my very trivial change makes it work exactly like it did before with no need for a workaround.

@dgbeck

I didn't get wind of the Backbone.$ = $ work around.. that would work for our case. Above work around also would work and would eliminate the need for hacking around this issue completely in some cases. For example when jquery is loaded from a CDN it would just work.

@akre54
Collaborator
@symmetriq

That won't work for Ender, unfortunately. module and module.exports both exist.

Having thought about this a bit more, I believe the Ender and Browserify contexts should both be treated as CommonJS hosts (which is exactly what they are). That being the case, the only real problem with the original is it assumes the CommonJS means there's no DOM library, and so I believe @dgbeck's suggestion is actually the right way to handle this (wherein we pass the same set of root DOM library references to the factory() call for the CommonJS context).

Unless there's a specific scenario in which it would cause a problem if those references are there?

Anyway, I'll submit a new patch shortly (just need to sync my fork).

Jason Sims added some commits
@akre54
Collaborator

the exports path is meant to be used with a commonjs module loader. Attempting to grab the DOM library from the global object is a step in the wrong direction and overloads the meaning of that check.

Can we assume that the common case is loading jQuery? If you are using another DOM library it's trivial to set Backbone.$ yourself.

@symmetriq symmetriq referenced this pull request
Closed

1.1.1 breaks browserify #2997

@jashkenas jashkenas added the change label
@jashkenas jashkenas closed this
@jashkenas jashkenas added the wontfix label
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 4, 2014
  1. Fix for Ender + Backbone

    Jason Sims authored
    When Backbone is used as an Ender module, `exports` always exists, so
    `root.ender` needs to be passed to `factory`. (When not using Ender,
    the 4th arg will be undefined, same as before.)
    
    Note that Ender 2.x no longer automatically integrates all modules into
    `$`. There are two ways to use Backbone with Ender 2:
    
    1. Assign it to its own var and use it like this:
    
    var Backbone = require('backbone');
    Backbone.View.extend({…});
    
    2. Integrate into `$` by adding `--integrate backbone` when building
    the Ender library, then use it like this:
    
    $.View.extend({…});
Commits on Apr 4, 2014
  1. Merge remote-tracking branch 'upstream/master'

    Jason Sims authored
  2. New fix for integrating with any common DOM library when Backbone is …

    Jason Sims authored
    …used as a CJS module (e.g. with Ender or Browserify)
This page is out of date. Refresh to see the latest.
Showing with 2 additions and 2 deletions.
  1. +2 −2 backbone.js
View
4 backbone.js
@@ -15,10 +15,10 @@
root.Backbone = factory(root, exports, _, $);
});
- // Next for Node.js or CommonJS. jQuery may not be needed as a module.
+ // Next for Node.js, CommonJS
} else if (typeof exports !== 'undefined') {
var _ = require('underscore');
- factory(root, exports, _);
+ factory(root, exports, _, (root.jQuery || root.Zepto || root.ender || root.$));
// Finally, as a browser global.
} else {
Something went wrong with that request. Please try again.