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

Make jquery in commonjs a standard dependency #3421

Closed
wants to merge 2 commits into from

Conversation

dgbeck
Copy link

@dgbeck dgbeck commented Dec 22, 2014

Context

Looks like this all started back in #2862, which added a UMD wrapper in which jquery was omitted as a dependency when a commonjs environment was detected, based on the reasoning that commonjs is for the server side and jquery is not needed on the server side. In this first iteration of the UMD, Backbone.$ was left undefined in the commonjs environment. However, commonjs was becoming increasingly common on the client side thanks to tools like browserify, Web Pack, component, cartero, et. al. #2862 was then merged so that backbone could be used with jquery in commonjs environments, which added a require( 'jquery' ) but wrapped it in a try / catch. However, this "light dependency" approach "broke" browserify, as discussed in #2997, since browserify would fail if jquery was not present. As a result of this issue the require + try / catch was removed altogether, and it was made a requirement to explicitly set Backbone.$ in all commonjs environments.

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

Unfortunately, we've seen this solution also has its share of problems:

Proposed solution

The proposed solution in this PR is to go with the straight forward solution of treating jquery as a standard dependency (as it is in AMD environments). What follows is a break down of how this solution would play out with the various use cases.

On the server side (node)

For people using jquery

Everything just works.

For people who do not need a DOM library

jquery will install and will load when the node application starts up, as soon as backbone is required, eventhough it is not needed. However both the disk space it consumes and the time it takes to load are too small to have any consequential impact.

For people who want to use a DOM library other than jquery

Same as above case, except that Backbone.$ will need to be set to the alternate DOM library during startup. (Assuming node_modules has been deduped, the backbone module is shared accross the entire application and thus setting Backbone.$ at application startup is sufficient to ensure that the alternate DOM library is always used by backbone.)

On the client side

For people using jquery

This is the most common case, I think. Using this approach, everything just works for this case as expected. (Note that npm dedupe still needs to be run to make sure that only one copy of jquery will be floating around.)

For people who want to use a DOM library other than jquery

This is the group of people who would be inconvenienced by this approach. Luckily the inconvenience is pretty minimal. They need to either

a) Explicitly set Backbone.$ to the DOM library of their choice whenever a page is loaded, and then configure the build tool to ignore the jquery dependency (so that jquery is not dead weight in their js bundle). With browserify, the exclude option can be used to have require('jquery') return undefined:

browserify main.js --exclude jquery > bundle.js

With Web Pack,

{
    plugins: [
        new webpack.IgnorePlugin(/^jquery$/)
    ]
}

b) configure their build tool to "alias" jquery for another library. With browserify, this can be done with aliasify,

npm install --save aliasify

Add an "aliasify" section to the application's package.json:

{
    "aliasify": {
        aliases: {
            "jquery": "zepto"
        }
    }
}

Using WebPack, the resolve.alias configuration option can be used:

{
    context: __dirname + "/app",
    entry: "./entry",
    output: {
        path: __dirname + "/dist",
        filename: "bundle.js"
    }
    resolve : {
        alias : {
            "jquery": "zepto"
        }
    }
}

For people who want to use a DOM library with backbone and jquery in other places

Just set Backbone.$ explicitly to the library of their choosing.

Comparison with try / catch

Another solution worth considering is to go back to #2862 which wraps the require( 'jquery' ) in a try / catch. This solution would also address most of the problems with the current approach. However,

  1. I don't see a significant upside over the standard dependency approach.
  2. 1.1.1 breaks browserify #2997 would again be a problem, which we know for sure, since we have been there before.
  3. The lack of clarity as to what is actually supposed to be at Backbone.$ presents a problem for module and meta framework authors. I agree with @akre54's comment:

Marionette should be using the same jQuery as Backbone and the same jQuery as your app. It becomes an absolute mess when your view's $el property has a different $.fn than the rest of your app

But where is the global jquery object? External modules and meta frameworks can not assume Backbone.$ is jquery and interact with it as such if there is no firm stance taken. What is Backbone.$ supposed to be in the baseline case? Is it jquery? Is it zepto? What the heck is it?

If backbone has a standard dependency on jquery then it is clear - the baseline case is that Backbone.$ === the global jquery object.

@jridgewell
Copy link
Collaborator

👍, though I think the try-catch is the better solution:

I don't see a significant upside over the standard dependency approach.

The biggest upside I as see is the server-side devs who don't need jQuery won't need to require it. It'll also allow both server/client devs to use the jQuery version appropriate to their app. As a side, requiring at least v2 is a problem.

#2997 would again be a problem, which we know for sure, since we have been there before.

When jQuery/Zepto are needed, this can be solved by the dev just using npm install --save jquery. If they want Zepto, just alias.

// package.json
// No need for aliasify
"browser": {
  "jquery": "node_modules/zepto/zepto.min.js"
}

When it isn't needed, it's solved with --exclude / --ignore / webpack.IgnorePlugin.

The lack of clarity as to what is actually supposed to be at Backbone.$ presents a problem for module and meta framework authors

I don't think adding jQuery to the package.json solves the real issue: using the global $ instead of Backbone.$. Package developers should not be using the global $. It's up to the dev to include jQuery (in which case, the try-catch would set Backbone.$) or manually include their preferred alternative.

@megawac
Copy link
Collaborator

megawac commented Dec 22, 2014

Another issue would be deciding which version of jquery to use -- 1.x vs 2.x

@@ -5,7 +5,8 @@
"keywords" : ["model", "view", "controller", "router", "server", "client", "browser"],
"author" : "Jeremy Ashkenas <[email protected]>",
"dependencies" : {
"underscore" : ">=1.6.0"
"underscore" : ">=1.6.0",
"jquery" : ">=2.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be like 1.10.x - 2.x or w.e., right? That way your app can determine which version of jQuery to use.

Copy link

Choose a reason for hiding this comment

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

yep it should be.

jquery 2.x only supports ie 9+ so this would be a pretty nasty breaking change to this lib for many people.

@jashkenas
Copy link
Owner

@dgbeck Want to amend your PR to include the proper version range?

@megawac
Copy link
Collaborator

megawac commented Feb 24, 2015

I'd say version range should be *. How about using a second file browser.js which sets Backbone.$ for you, this way loading jquery won't be necessary on node

@akre54
Copy link
Collaborator

akre54 commented Feb 24, 2015

Well, not every jQuery supports exports. As far as I can tell it was only added in 1.10.0/2.0.0.

this way loading jquery won't be necessary on node

Is it that big of an issue to load jQuery in Node? Perhaps it's slightly wasteful, but otherwise it should be harmless and side effect-free.

@jridgewell
Copy link
Collaborator

See #3421 (comment). I think the original try-catch was the perfect solution, we just need to document how to exclude jQuery when it's unneeded or how to alias/remap it to another dependency when something else is needed.

@akre54
Copy link
Collaborator

akre54 commented Feb 24, 2015

But server side doesn't really have code-size issues the way client-side does. Who cares if you load jQuery on the server when you don't use it?

@jridgewell
Copy link
Collaborator

Who cares if you load jQuery on the server when you don't use it?

No argument against it.

@jashkenas
Copy link
Owner

It's an extra two megabytes in every npm-installed copy of Backbone.

Given that even dev Backbone is just a 60k script — that's a little bit nasty.

@megawac
Copy link
Collaborator

megawac commented Feb 24, 2015

I agree with @jashkenas, we don't need to make jQuery a hard npm dependency but importing with try { } catch() {} should still work if the user sets $ as a dependency in their project (parent node_module folder) right?

@samccone
Copy link

Yep, people just have to manually include the two megabytes now.

The volume of people that are confused by this, and the "anti-pattern" that backbone uses as far as the jquery dependency goes is really unfortunate.

I get it though, developers should be good at debugging so they should just figure it out, and figure out what flavor of shim they need to get around this design decision.


Most of backbone works without jquery, and I get that so why force people to download jquery?

There are entire layers of the library that will not work without a jquery like library. So if someone was to npm install it, use it in their app, and used any of the pieces that depend on a jquery like object they would run into problems, because things would not work.

Dependencies are things that a project depends on to work, thus a jquery like object is a dependency.

Can we choose some "light" dom lib that will make all of bb pass and just use that? (and specify it as a dependency)

@dgbeck
Copy link
Author

dgbeck commented Feb 25, 2015

Sorry, just saw the recent activity here this afternoon.

It looks like the first version of jquery available on npm was 1.5.1, and that version supports exports. However I think using "*" as @megawac suggested makes more sense. (I could not find a minimum required version of jquery in the backbone docs.) This PR has been updated with a wildcard dependency, but I'm happy to change it to whatever is requested.

In terms of to try / catch or not to try / catch, either way is a step up from where we are now.

But to play with some numbers, if we were to go with the try / catch approach, say around half of all people using commonjs on the client side would have to spend some time debugging why their build system is failing with an out of the box version of backbone -- all those people that install backbone and try to compile their assets without installing jquery will run into this problem.

On the other hand, if we go with the standard dependency approach, I will venture to say that at worst 5% of people who are in the (smaller) group of server-side devs using backbone would be annoyed that there is a jquery dep that is not being used, and they would get over it quickly, without having to invest any time in debugging anything. Yes, 2 megs is big relative to backbone, but it is not a significant portion of a modern web application.

In the interest of a thorough exploration, there is an additional con to the standard dependency approach worth reiterating, although it falls more in npm's court to resolve than it does in backbone's. If jquery is a standard dependency of backbone, and backbone is installed first, and then jquery is installed on the app level later, there will be two copies of jquery, unless npm dedupe is run. For instance

npm install --save backbone
npm install --save jquery

Now we have

app
   backbone
      jquery
      underscore
   jquery

This is an unfortunate situation. As already discussed in this thread, having multiple copies of jquery floating around is likely to cause problems. Plugins installed on one jquery instance are not available on another, data attached to DOM nodes using the $.data method of one jquery instance is not accessible when using the other instance, etc. npm dedupe solves the issue, but not everybody runs that command. The good news is that npm 3, very quickly approaching release, is going to fix this issue, running a dedupe on every npm install.

All in all, the standard dependency approach seems like the more solid long term solution.

@jashkenas
Copy link
Owner

Thinking this through again, I don't think we should change this at the moment. Perhaps at a future date. Why?

  • Despite the rise of alterna-npm client-side loaders, npm is still first and foremost Node's package manager.
  • "Real" JavaScript modules in ES20Whatever won't be "CommonJS", they'll be something else.
  • Backbone.$ = require('jquery') really isn't very much to ask.
  • "How do I stop Backbone from loading jQuery when I don't want it?" Would be much harder to answer.

@jamesplease
Copy link
Contributor

😑

@akre54
Copy link
Collaborator

akre54 commented Mar 20, 2015

Despite the rise of alterna-npm client-side loaders, npm is still first and foremost Node's package manager.

Agreed. But people are using it for client-side now. And it will get better in npm land.

"Real" JavaScript modules in ES20Whatever won't be "CommonJS", they'll be something else.

CommonJS is here, and people are using it and have been using it for years. imports is a syntax addition that will require Backbone to be transpiled to be used anywhere else. We can always add System or whatever support when that actually lands.

Backbone.$ = require('jquery') really isn't very much to ask.

Yes it is. It's non-obvious, and breaks in CommonJS environments where you can't modify modules (#3156.).

"How do I stop Backbone from loading jQuery when I don't want it?" Would be much harder to answer.

When would this happen? So long as we don't add the dependency to Backbone's package.json we should be fine. If you need it, you add it to your app's package.json. If you don't, don't. It just works.

I appreciate you taking a decisive stance on this, I just happen to think it's the wrong one.

@samccone
Copy link

:|

@jashkenas
Copy link
Owner

Oh, I'm sorry — I was looking at this pull request, which does add jquery to package.json.

If we're not going to do that, then this PR needs to be changed.

@jashkenas jashkenas reopened this Mar 20, 2015
@akre54
Copy link
Collaborator

akre54 commented Mar 20, 2015

Thanks for reevaluating.

Ok so game plan: @dgbeck remove jquery dependency from package.json and wrap the jQuery require in a try...catch. Right?

@jashkenas
Copy link
Owner

Doesn't the try/catch break many of the commonjs tools that brokenly try to statically analyze your script for require statements?

@akre54
Copy link
Collaborator

akre54 commented Mar 20, 2015

As far as I'm aware that's mostly with browserify. You tell it to ignore or exclude jquery and it works fine. #2997 (comment)

@jridgewell
Copy link
Collaborator

I think the try-catch is the correct solution as well. #3421 (comment)

@jashkenas
Copy link
Owner

Okay then. Let's either amend this pull request, or close it in favor of an agreed-upon one.

@akre54
Copy link
Collaborator

akre54 commented Mar 20, 2015

To get the ball rolling: #3542.

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.

7 participants