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

AMD boilerplate still defines jQuery as a dependency #3074

Closed
eastridge opened this issue Mar 19, 2014 · 21 comments
Closed

AMD boilerplate still defines jQuery as a dependency #3074

eastridge opened this issue Mar 19, 2014 · 21 comments

Comments

@eastridge
Copy link
Contributor

@akre54 I'm trying to get NativeView to play nicely with Handlebones. Non AMD tests are all passing. Hoorah!

The current AMD setup in backbone.js still defines jQuery as a hard dependency:

https://github.com/jashkenas/backbone/blob/master/backbone.js#L12

Which will cause my AMD build to blow up when I try to test it. Any suggestions as to how this can be handled by Backbone? I'm sure there was much strife over the AMD boilerplate and getting it in, but boy I wish it wasn't there at all. There was nothing wrong with shimming it IMHO.

@akre54
Copy link
Collaborator

akre54 commented Mar 19, 2014

Shimming messes up the build tool IIRC. The commonjs solution is to assign
Backbone.$ directly, which may be the route we take here.

Opinions on killing the jquery require for AMD?

@eastridge
Copy link
Contributor Author

Both scenarios seem pretty crappy:

  1. Leave as is. This means I need a map in require that maps jQuery to an empty object. Or maybe some require black magic that I do not yet know. @jrburke any ideas on how to achieve a conditional dependency without a plugin? i.e. we don't care if it comes back null?

  2. Remove jQuery as a dep. This means I need to set $ manually as I setup my app correct?

Am I missing a clever third solution? Option 1 seems more correct in that the whole point of #3003 was to no longer require jQuery, so may as well reflect that in the defined dependency tree.

@jrburke
Copy link
Contributor

jrburke commented Mar 19, 2014

I do not have all the context, but either Backbone depends on jQuery or it doesn't. All module systems, AMD, CommonJS/Node, even ES6 are based on a similar principle for dependencies. I believe this sort of issue came up for the Node pathway in Backbone too.

Perhaps if there are parts of Backbone that need jQuery that should be broken out as a separate module/file/addon. But I do not have enough context to suggest a final pathway, best that the folks in the Backbone community sort it out.

@magalhas
Copy link

I don't mind setting up Backbone.$ on the application setup, but that's just my opinion. One solution in the AMD scenario is catching the error for not finding jquery. Though it's ugly to do that.

@eastridge
Copy link
Contributor Author

Have an open PR:

#3075

@akre54
Copy link
Collaborator

akre54 commented Mar 19, 2014

either Backbone depends on jQuery or it doesn't

Nope. As the docs state, Backbone's only hard dependency is Underscore. It has a "light dependency" on jQuery that can be overridden. One of the many advantages of a late-binding language like JS is rewriting dependencies or exposed modules on the fly, which is particularly useful for scenarios like this where the code remains functionally similar but has large underlying portions replaced.

Perhaps if there are parts of Backbone that need jQuery that should be broken out as a separate module/file/addon

Also nope. Backbone shouldn't need to use AMD to manage internal code.

I believe this sort of issue came up for the Node pathway in Backbone too.

Not in the node pathway, the Browserify pathway. Browserify was choking on a try-catch wrapped around the commonjs require. The difference between node and Browserify is that the former can do runtime lookups, while the latter must do a precompilation step.

I'm not hugely opposed to making the dev set Backbone.$ manually, but it is a bit of an ugly hurdle for newbies. If we make the change in #3075 let's make sure to document the hell out of it.

@braddunbar
Copy link
Collaborator

either Backbone depends on jQuery or it doesn't

Nope. As the docs state, Backbone's only hard dependency is Underscore. It has a "light dependency" on jQuery that can be overridden.

I think @jrburke means that a light dependency is not a dependency, which is the consensus regarding commonjs as well.

I believe this sort of issue came up for the Node pathway in Backbone too.

Not in the node pathway, the Browserify pathway.

Pedantically, it's commonjs in both cases. Regardless, it's precedent we should heed.

If we make the change in #3075 let's make sure to document the hell out of it.

👍

@akre54
Copy link
Collaborator

akre54 commented Mar 19, 2014

Pedantically, it's commonjs in both cases. Regardless, it's precedent we should heed.

They're both commonjs, but the difference is runtime lookup vs a precompile step. The former you can use a try...catch. The latter you can't.

I've seen projects use require within methods for runtime dependencies, both for lazy-loading, and for avoiding dependency loops (i.e. utils depends on helpers which has a method depending on a method from utils).

@jrburke
Copy link
Contributor

jrburke commented Mar 19, 2014

Sorry if my previous answer was a bit to short, some more follow up:

ES modules will work like AMD as far as behavior of specifying dependencies: they need to be specified more declaratively, up front. A dynamic dependency can be loaded via System.import('jquery').then(function(){}), similar to require(['jquery'], function()) in AMD, but it is an async, callback mechanism.

This is different from Node's commonjs-like system because they have synchronous file access, so they can support a dynamically fetched require('jquery') and throw. That is a very bad practice in the browser though, so it does not carry forward to ES6.

End result: deciding now how Backbone would like to express its dependencies for AMD will mean it has solved its solution for ES6 modules, whatever that solution is, even if it is "do nothing".

If you wanted to keep "jquery" as a specified dependency, for AMD projects that did not want to use any DOM library helper, then instructing AMD users to do the following after their config setup would allow that to work:

// Showing requirejs config call here, but same holds for any AMD
// loader config call.
requirejs.config({
  // normal config here
});

// Seed the module loader with an empty jQuery dependency. Just
// do this before starting main app loading
define('jquery', function(){});

// Now do app loading
require(['app/main']);

The other option is to just save that define(function(){}) in a file called jquery.js in the project.

My impression is that most Backbone users using Backbone in the browser will be using it with a DOM helper library (zepto/ender can be supported via an AMD loader map config that points 'jquery' to those providers).

If that is true, then I would bias the dependency expression to mention 'jquery' and suggest the pathways mentioned above for AMD users that do not want a DOM helper library with Backbone, but you all know your community better.

@jashkenas
Copy link
Owner

If that is true, then I would bias the dependency expression to mention 'jquery' and suggest the pathways mentioned above for AMD users that do not want a DOM helper library with Backbone

Sounds good to me. @jrburke knows best ;)

@akre54
Copy link
Collaborator

akre54 commented Mar 19, 2014

👍 to setting an empty jquery object.

@eastridge
Copy link
Contributor Author

@akre54 so the actionable item here would be to keep jQuery as a dep in the AMD definition, then change Backbone's AMD test definition to setup a blank jQuery object if I am reading correctly?

@magalhas
Copy link

I must say it would feel strange if someone wants to use a library other than jQuery in Backbone.$ and having to create a jquery file/definition.Cumprimentos,
José Magalhães

On Wed, Mar 19, 2014 at 5:31 PM, Ryan Eastridge notifications@github.com
wrote:

@akre54 so the actionable item here would be to keep jQuery as a dep in the AMD definition, then change Backbone's AMD test definition to setup a blank jQuery object if I am reading correctly?

Reply to this email directly or view it on GitHub:
#3074 (comment)

@eastridge
Copy link
Contributor Author

@magalhas I agree. I already primarily use zepto, and seems a little strange

@akre54
Copy link
Collaborator

akre54 commented Mar 19, 2014

Agreed with @magalhas. All seem like crap solutions.

@eastridge
Copy link
Contributor Author

@akre54 so is it simple enough to say that there are only two reasonable solutions:

  1. Remove jQuery AMD dependency #3075
  2. or defining jQuery one way or another (even if empty object) if using AMD

Does this capture the conversation to date correctly? @jrburke thanks for your input on this. All hail Lord Require =)

@eastridge
Copy link
Contributor Author

Oh and if it's #3075 let's make it a minor release and not a patch release. 1.1.2 blew up in our faces in a way that I would not have expected a patch release to have.

@magalhas
Copy link

I kind of prefer #3075 (dependency removal) and also this way it works the same way as CJS module definition do. As said the downside is for newbies that won't know that Backbone.$ needs to be defined in order to have views and routers working properly, though that should be overcome with documentation. This will break the current "interface" so a minor release would be proper in my opinion.

@magalhas
Copy link

Also on a side note, we could have a default Backbone.$ implementation that throws an error stating that a library should be "plugged in", that would help out people that doesn't read documentation at all :)

UPDATE Though things like if (!Backbone.$) Backbone.$ = ... would stop working :|

@eastridge
Copy link
Contributor Author

Reopening as it seems that this will require more discussion.

@eastridge eastridge reopened this Mar 19, 2014
@jashkenas
Copy link
Owner

@jrburke says to just shim jquery with whatever you like for AMD ... so just shim jquery.

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

Successfully merging a pull request may close this issue.

6 participants