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

Add optional support for require.js #20

Closed
jpmckinney opened this issue Jul 4, 2012 · 13 comments
Closed

Add optional support for require.js #20

jpmckinney opened this issue Jul 4, 2012 · 13 comments
Labels

Comments

@jpmckinney
Copy link
Contributor

If people want to manage dependencies with require.js, we can add the necessary code to assist integration. Think about using it in the demo and tutorial.

@jpmckinney
Copy link
Contributor Author

We would want to namespace require to avoid conflicts. See http://requirejs.org/docs/faq-advanced.html and http://requirejs.org/docs/faq-optimization.html for options.

@jpmckinney
Copy link
Contributor Author

Would have to wrap all files in define blocks. In order for require.js to be optional, we would have to supply dummy implementations of define and require. It looks like require.js handles multiple files having the same dependencies just fine. The strategy would be for each non-abstract widget to define all its dependencies, so that users can just include the widgets and not worry about core files.

Try implementing this strategy in the demo site to see if it works.

@jpmckinney
Copy link
Contributor Author

See the requirejs branch. Verdict is that requirejs will be too confusing to beginners and to people who don't need it. script tags is best for beginners, and advanced users should just package/compile assets in production. requirejs just gets in the way during development, with its need to cache bust for example. Perhaps worst of all, it seems impossible to use requirejs without adding define calls to every dependency, which means everyone needs to buy into requirejs for this to work.

@dergachev
Copy link

James, have you considered ypenope.js? I found it a lot simpler than requirejs.
http://yepnopejs.com/

@jpmckinney
Copy link
Contributor Author

It is indeed simpler. I tried it in a new yepnope branch. It seems to download all the dependencies correctly, but when it executes the complete functions, it starts with the one in reuters.1.js (which breaks, because it expects all the other complete functions to have run first). I would expect yepnope to run the complete functions in an order that respects the dependency tree: i.e. run AbstractManager.js's complete, then Manager.jquery.js's then reuters.1.js. I'll try labjs, see if it works better.

@jpmckinney
Copy link
Contributor Author

I created an issue on yepnope: SlexAxton/yepnope.js#161

@jpmckinney
Copy link
Contributor Author

I also created an issue with headjs: headjs/headjs#231

Amazingly, the very old, no longer developed labjs works fine!

@jpmckinney
Copy link
Contributor Author

Anyway, for the demo, I think <script> tags still make the most sense. No need to educate people about script loaders to teach them ajax-solr.

If we were to integrate LAB for use by developers outside the demo, developers would need to set the LAB base path to the folder containing ajax-solr, because the paths to ajax-solr files are hardcoded:

$LAB.setGlobalDefaults({BasePath: '../../'});

All their other scripts would have to be relative to this base path. If another library required developers to set the LAB base path, it'd be impossible to satisfy both. This wouldn't be an issue if LAB had something like Yepnope's path filter. The solution to the conflict would be to define a AjaxSolrBasePath global instead of using $LAB.setGlobalDefaults (which is what I do in the labjs branch).

Anyway, I'm not sure LABjs is popular enough to bother adding this to ajax-solr. The only request has been for requirejs so far. If Yepnope or headjs fix their issues, we can maybe add those later.

@jpmckinney
Copy link
Contributor Author

I tried require.js again, without using define to create modules this time. It has the same bug as yepnope and head.js requirejs/requirejs#710 Maybe they consider it a feature?

@jpmckinney
Copy link
Contributor Author

With some help from the maintainer, I got head.js working. See this fiddle for a small example: http://jsfiddle.net/TWDy4/2/

Update: The approach becomes very unwieldy, as you need to nest head calls for each dependency. See e.g. reuters.3.js

@jpmckinney
Copy link
Contributor Author

requirejs works now - I had the wrong syntax.

@dergachev
Copy link

James, I only quickly skimmed over your commits, but one thing stands out:
Does it make sense to call define twice in AbstractSpatialWidget.js?

https://github.com/evolvingweb/ajax-solr/blob/requirejs/core/AbstractSpatialWidget.js#L67

@dergachev dergachev reopened this Apr 16, 2013
@jpmckinney
Copy link
Contributor Author

Oops, that was leftover from my earlier work. Keen eye! I've removed it now.

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

No branches or pull requests

2 participants