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

Remove unnecessary jquery dependency in package.json #978

Merged
merged 1 commit into from
Mar 20, 2014

Conversation

wbinnssmith
Copy link
Contributor

Since Marionette simply uses Backbone.$, and doesn't explicitly require('jquery'), this dependency is no longer needed in the package json.

Also, the wreqr and babysitter versions referenced here each pull in their own jquery as well. Updating to the latest versions of these libraries which don't include jquery (for the same reasons) would be great.

@jamesplease
Copy link
Member

This was just added back after some discussion, though it's still a pretty hot topic.

You're right that Backbone depends on jQuery, but bower doesn't seem to pull dependencies of dependencies. If you remove the jquery dependency, rm -r bower_components, then run the tests they'll fail because jQuery is nowhere to be found.

I have memories of there being other issues with removing jQuery, too. I'm receptive to arguments about removing jQuery as a dependency, but I'm unsure if it's possible. I'll keep this open for more discussion.

@wbinnssmith
Copy link
Contributor Author

This actually refers to package.json. In npm's case with wreqr and babysitter, it pulls in jquery 1.8.3, the npm version of which pulls in a bunch of stuff like jsdom and contextify, resulting in browserify builds needing to compile native c extensions in node just for doing frontend dependency management.

Additionally, the jquery dependency will never actually be bundled in the build either, since there isn't an explicit `require('jquery') call anyway. See @substack's feedback in the original Backbone PR for more context. He may be able to chime in here as well.

@jamesplease
Copy link
Member

@wbinnssmith
Copy link
Contributor Author

Neither marionette, nor wreqr or babysitter explicitly require('jquery'), so based on substack's comment, it doesn't have a place in package.json. Yeah?

@jamesplease
Copy link
Member

Believe me you, I've been persuaded. I'm just going to look through the issues to see others' reasons for including it back in before resigning myself to getting rid of it again.

I'd also like to hear @samccone / @cobbweb // @rhubarbselleven 's thoughts.

The require() test is good for determining the dependencies for node modules, but it doesn't seem as clear to me when something is a bower dependency or not.

@wbinnssmith
Copy link
Contributor Author

Awesome. Seriously appreciate it!

Any chance of getting wreqr and babysitter bumped and depended on by marionette? They had their jquery dependencies removed (by me 😉) last week or so.

@samccone
Copy link
Member

samccone commented Mar 4, 2014

Ok basically my point is this. Because backbone is screwing us over, we need to polyfill this requirement. Without this you will not be able to use marionette with tools like browserify.

WHEN backbone fixes this we can drop this requirement.

@samccone samccone closed this Mar 4, 2014
@jamesplease
Copy link
Member

@wbinnssmith, to elaborate a bit more, from my understanding it seems like Browserify works through the dependency chain of the package.json file. Marionette ultimately does need jQuery, but, you're right, it should technically come from Backbone. Since jQuery is optional with Backbone & absent from its package.json, it isn't a specified as a dependency there so it must be one here.

This situation seems like a corollary to @substack's determination of whether something is a dependency or not, and not completely at odds with it.

@wbinnssmith
Copy link
Contributor Author

Just so you guys know, the copy of jquery referenced in package json will
never be included in a browserify or webpack bundle, or read in by node
since there is no explicit require. Unfortunately including it is nothing
but wasted bytes for these users.

@bonobos is now maintaining a fork of marionette to address this if anybody
using commonjs comes across this.

Thanks for your consideration and appreciate the followup Jmeas.

On Mar 4, 2014 4:08 PM, "Jmeas Smith" notifications@github.com wrote:

@wbinnssmith, to elaborate a bit more, from my understanding it seems
like Browserify works through the dependency chain of the package.json
file. Marionette ultimately does need jQuery, but, you're right, it is from
Backbone. Since jQuery is technically optional with Backbone & absent from
its package.json, it isn't a specified as a dependency there so it needs to
be one here.

This situation seems like a corollary to @substack's determination of
whether something is a dependency or not, and not completely at ends with
it.

Reply to this email directly or view it on GitHub.

@jamesplease
Copy link
Member

@wbinnssmith, I appreciate your persistence! I know nothing about browserify, so unfortunately my opinions here aren't worth much. Sometime soon-ish I'll take a look at setting up Marionette with Browserify and see what I come up. @samccone did this recently to come to his conclusion that it's a necessary dependency..but perhaps there's another way!

@wbinnssmith
Copy link
Contributor Author

For sure. Let me know if there's anything I can do to help!

I can update the wiki page on using marionette with browserify, since it
seems a bit outdated and recommends shimming, which isn't necessary anymore
since the latest jquery supports commonjs, etc.

On Wednesday, March 5, 2014, Jmeas Smith notifications@github.com wrote:

@wbinnssmith https://github.com/wbinnssmith, I appreciate your
persistence! I know nothing about browserify, so unfortunately my opinions
here aren't worth much. Sometime soon-ish I'll take a look at setting up
Marionette with Browserify and see what I come up. @samcconehttps://github.com/samcconedid this recently to come to his conclusion that it's a necessary
dependency..but perhaps there's another way!

Reply to this email directly or view it on GitHubhttps://github.com//pull/978#issuecomment-36815086
.

@jamesplease
Copy link
Member

Well, the issue at hand is whether or not the dependency is required for, at the least, Browserify. If you could make an example app with Browserify that depends on Backbone+@bonobos' Marionette fork, then that would be a good step to convincing us that it isn't necessary.

At least, that's the issue as far as I understand it. //cc @samccone

@samccone samccone reopened this Mar 6, 2014
@samccone
Copy link
Member

samccone commented Mar 6, 2014

k @wbinnssmith so you are right, marionette does not have an explicit require for $, however backbone does. Yet backbone does not have jquery in its package.json. So at some point you do have to do something like this https://github.com/samccone/marionette-browserify/blob/master/marionette_shim.js

That being said, including jquery in marionette's package.json is more of a formality because you do need it, at least until backbone fixes this issue.

Is something I am saying wrong? Or not clear?

@jamesplease
Copy link
Member

@wbinnssmith, we've mentioned a few times how @samccone made an example app with Browserify to justify our decision to include jQuery as a dependency. We made it public so you can take a look. If you can fork that and make it work without jQuery as a dependency in Marionette's manifest then that would be enough to convince us to remove it.

I must stress that I know nothing about Browserify, so I'm just relaying messages here. I could take a look to figure out myself but I won't be able to for some time :)

@jamesplease
Copy link
Member

ping @wbinnssmith

@wbinnssmith
Copy link
Contributor Author

All right guys, take a look here.

jquery is yanked up to the application and is no longer a dependency of marionette or any of its dependencies. jquery also becomes the only global on the page, though it could also be eliminated through something like browserify-shim and set as require('backbone').$ = require('jquery').

this is the exact approach (and the same fork of marionette) that powers AYR from us Bonobos folks.

here's the output of npm ls jquery:

browserify [master] % npm ls jquery
marionette-browserify@ /Users/will/src/marionette-integrations/browserify
└── jquery@2.1.0

@samccone
Copy link
Member

This has been on mind for awhile @wbinnssmith , So I am on the same page as you now. Like I said above marionette uses backbone and backbone requires a global $ variable to be somewhere in the namespace. Since backbone is lame and does not require jquery in its package.json we have it in ours.

This will go into the 1.7 branch.

Also if you would like could you open a PR into marionette integrations.

Thanks for your work, also now you will not need to use a fork of marionette.

@samccone samccone added this to the 1.7.0 milestone Mar 20, 2014
@jamesplease
Copy link
Member

These are convincing arguments, @samccone, but before we merge let me raise some counter arguments for our consideration. It will have to wait until tonight after work though :)

@jamesplease
Copy link
Member

First I'll make some arguments for removing jQuery, then some for keeping it in response to getting rid of it

Remove jQuery

  1. Marionette does not require jQuery anywhere, therefore it isn't actually a dependency, right?
  2. Marionette pulls in Backbone's $. Backbone should be pulling in jQuery, but it isn't our fault that it isn't. Therefore we should remove it, and maybe pressure Backbone to include it.

Keep jQuery

  1. True, but Marionette is also a bower component, and bower components have no notion of requiring. With this in mind you might be more willing to say that jQuery is actually a dependency of the Marionette bower component, as Marionette clearly uses jQuery-specific methods. If this is true, then maybe you'd want to add it to the bower manifest, then follow by adding it to the npm manifest for consistency. This argument is pretty weak, I admit; the second point above is probably strong enough to rule this out.
  2. So let's examine that second point. The crux of an argument against this point is that jQuery is not really a dependency of Backbone, but it is a dependency of Marionette. This is how I can see someone coming to this conclusion:

From this, we can gather that Backbone is barely depending on that $ variable. It depends so little on it, in fact, that it is pretty reasonable for them to not include it as a dependency and give instructions on how to replace it with similar APIs like Zepto.

This makes a lot of sense, too, when you think about how durn simple Backbone's views are. Backbone hardly does anything at all with the DOM. And this is precisely why Marionette is so convenient, because it fills in that missing gap by providing more robust view classes to work from.

In doing this Marionette makes a much stronger stance in requiring jQuery, as it utilizes quite a number of jQuery-specific functions to control its views.

So I can see how someone would use these arguments to say, 'Backbone doesn't really depend on jQuery, but Marionette does.'

With that said, I'm actually fine with removing it. I'm particularly fine with removing it if all of those jQuery functions linked above have Zepto equivalents, in which case Marionette is just as agnostic about its $ as Backbone is (in principle, not quantity). I just wanted to play devil's advocate here.

@jamesplease
Copy link
Member

Spoke to @samccone who still thinks that it should be removed.

Hasta luego, jQuery!

jamesplease added a commit that referenced this pull request Mar 20, 2014
Remove unnecessary jquery dependency in package.json
@jamesplease jamesplease merged commit 7635799 into marionettejs:master Mar 20, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants