-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Remove unnecessary jquery dependency in package.json #978
Conversation
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, 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. |
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. |
Neither marionette, nor wreqr or babysitter explicitly |
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 |
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. |
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. |
@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. |
Just so you guys know, the copy of jquery referenced in package json will @bonobos is now maintaining a fork of marionette to address this if anybody Thanks for your consideration and appreciate the followup Jmeas. On Mar 4, 2014 4:08 PM, "Jmeas Smith" notifications@github.com wrote:
|
@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! |
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 On Wednesday, March 5, 2014, Jmeas Smith notifications@github.com wrote:
|
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 |
k @wbinnssmith so you are right, marionette does not have an explicit require for $, however backbone does. Yet backbone does not have 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? |
@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 :) |
ping @wbinnssmith |
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 this is the exact approach (and the same fork of marionette) that powers AYR from us Bonobos folks. here's the output of
|
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 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. |
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 :) |
First I'll make some arguments for removing jQuery, then some for keeping it in response to getting rid of it Remove jQuery
Keep jQuery
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. |
Spoke to @samccone who still thinks that it should be removed. Hasta luego, jQuery! |
Remove unnecessary jquery dependency in package.json
Since Marionette simply uses
Backbone.$
, and doesn't explicitlyrequire('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.