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

Replace Backbone.$ with Marionette.$ #1844

Closed
samccone opened this issue Sep 9, 2014 · 8 comments
Closed

Replace Backbone.$ with Marionette.$ #1844

samccone opened this issue Sep 9, 2014 · 8 comments

Comments

@samccone
Copy link
Member

samccone commented Sep 9, 2014

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
Copy link
Member

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
Copy link
Member

cmaher commented Sep 9, 2014

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

  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
Copy link
Member

cmaher commented Sep 10, 2014

@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 #980 (comment) (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
Copy link
Member

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants