search

marionettejs / backbone.marionette

The Backbone Framework
https://marionettejs.com
Other
7.06k stars 1.26k forks source link

Replace Backbone.$ with Marionette.$ #1844

Closed samccone closed 10 years ago

samccone commented 10 years ago

We can also drop

Backbone.$.Deferred

this change means that once again jquery is a dep of marionette, except for the fact that every where that we are going to use $ we will use Marionette.$

jamesplease commented 10 years ago

I like this in theory. Backbone won't ever specify jQuery as a dependency, but it is absolutely a dependency of Marionette.

With that said, there's been a ton of heated discussion over this in the past. Before we act, we should understand why people were so fervently against us specifying jQuery as a dependency of Marionette, and make sure we're not breaking anything by doing it.

The most important things to consider are CommonJS/AMD builds.

cmaher commented 10 years ago

If anyone is replacing jquery with something like zepto, they can just alias it. It's the same thing as replacing underscore with lodash.

jamesplease commented 10 years ago

So I dug through old issues (this is the most important one) and determined that the main reason we removed jQuery from the npm dependencies is because nowhere do we have require('jquery'); in the source.

If we choose to add it back as a dependency, then it only makes sense that we also add in that line of code.

I think this is a really good idea. Firstly, Backbone doesn't depend on jQuery at all. So much so that they recently abstracted all of the jQuery-dependent code in helpers to ease your transition away from using it. And they've infamously neglected to include it as a dependency in the package.json, much to the annoyance of CommonJS users everywhere.

However, Marionette does depend on jQuery. Very much so, in fact, and it is highly unlikely that this will change anytime soon.

Because of this, I think we should make the step to add:

Marionette.$ = require('jquery');

within Marionette, and then specify it as a dependency.

@wbinnssmith, I'd like to get your thoughts on this change.

jasonLaster commented 10 years ago

Two questions:

  1. will this make it more annoying for react people to integrate?
  2. will this mean people who load backbone separately now get it twice?
jamesplease commented 10 years ago
  1. Good question. @thejameskyle might have more to say about this? I'm not too familiar with React.
  2. Yup, unless they make sure that their jQuery dependency matches the range that we provide in Marionette. There's not too much we can do about this, I think. Folks who use CommonJS should understand how it works before they use it. It's not our job to protect them from a lack of understanding of the tools, I think.
cmaher commented 10 years ago

@jasonLaster as it is right now, I think it's very difficult for react people to integrate. In 2.3, along with this jquery requirement, I think we can separate out all of our DOM interaction to 9 methods (plus Deferred), detailed at the end of https://github.com/marionettejs/backbone.marionette/issues/980#issuecomment-51868548 (Marionette.DOM?) that people can override to use different engines. With that, we'd be like Backbone, where the default implementation requires jQuery, but, unlike Backbone, we could make the project work out of the box and not confuse people

jamesplease commented 10 years ago

If we agree that this is a major change – it sounds like one to me – then we can close this issue and add it to #1796. The discussion can continue in this thread even after its closed.

What do y'all think?

jamesplease commented 10 years ago

Mmk, well, I think it's breaking. And I don't want to piss anyone off by releasing this in 2.3, so I vote that we wait until v3 at the earliest. I've moved this to #1796, and I'm closing the issue in the meantime.

The conversation can continue here.