Fixed version of gem that calculate dependensies in proper way #10

Merged
merged 9 commits into from Oct 12, 2012

Conversation

Projects
None yet
8 participants
Contributor

egorkhmelev commented Feb 2, 2012

I have found that current version of gem tried to resolve and append files based on goog.require directive that actually works not so stright. For example if you will try goog.require('goog.fx'); you will got an error that file goog/fx/Animation/EventType.js (one of the dependensies) does not exist and its true, it was provided by file goog/fx/animation.js

Here is corrected version that use calcedeps.py script to calculate properly deps and include them in project.
However its a bit harder coz it requires new directive. I hope there are some way to do it easier.

Owner

igrigorik commented Feb 4, 2012

Hmm, how does this work in terms of performance? Last time I tried this, it was very very painfully slow when I had to shell out to python. Ideally, we'd want to just rewrite the logic in Ruby.

Contributor

egorkhmelev commented Feb 4, 2012

Locally it works perfect. And with sprockets default caching even better.
I'm developing now with it and don't experiencing any performance issues.

Anyway its fast fix because current version doesn't work at all with something except good.dom

Egor Khmelev
Front-end and Mobile devices
bookmate.com/khmelev (http://bookmate.com/khmelev)

On суббота, 4 февраля 2012 г. at 22:33, Ilya Grigorik wrote:

Hmm, how does this work in terms of performance? Last time I tried this, it was very very painfully slow when I had to shell out to python. Ideally, we'd want to just rewrite the logic in Ruby.


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

lypanov commented Feb 15, 2012

It doesn't have to be fast.

My workaround for this issue several months back was to simply enable the javascript side closure deps calculation when in development mode and when in precompile mode disable and run the python.

- end
+class ClosureDirectiveProcessor < Sprockets::DirectiveProcessor
+ def process_require_closure_directive(path)
+ # Locate python script that helps us to calculate deps
@igrigorik

igrigorik Feb 23, 2012

Owner

Should this be a configuration option? I'm not familiar enough with differences between closurebuilder.py and calcdeps.py.

@joe1chen

joe1chen Feb 23, 2012

calcdeps.py is "super-legacy" and has been replaced by closurebuilder.py.

See:
https://docs.google.com/present/view?id=dgsp8w6z_72fxtnnmdb&start=18
http://code.google.com/closure/library/docs/closurebuilder.html

Definitely should not be using calcdeps.py.

@igrigorik

igrigorik Feb 23, 2012

Owner

Yikes, and that's a slide deck from 2010! Well, good to know.. I guess they should update the docs to reflect that.

In any case, if we know that calcdeps is not the right tool, seems like we should be defaulting the user to closurebuilder?

@egorkhmelev

egorkhmelev Feb 23, 2012

Contributor

Yes, thats true, calcdeps is deprecated, but there are problem with closurebuilder, it requires from user to create own namespace and not gonna work without it (your example for this project not gonna work with closurebuilder). It might be not clear for end user who not so familar with closure details, and even for advanced closure users. You see, I left closurebuilder code in comments, coz I've started with it.

Moreover, I've changed regexp in calcdeps a bit to handle files like *.js.erb
So I think its not a good idea to give a user option to point on py script.

@igrigorik

igrigorik Feb 23, 2012

Owner

Ok, so I'm not following the logic yet.. Closurebuilder is more complicated but is supposedly the recommended approach, while calcdeps is the old and tested way. Judging by what you're saying, it seems like there would definitely be (more advanced) users that should be using closurebuilder, in which case, we can default to calcdeps, but provide an override option to use closurebuilder instead - no?

@egorkhmelev

egorkhmelev Feb 23, 2012

Contributor

Yes, good idea. But actually closurebuilder should be changed as well, to handle *.js.erb files. So we need to keep both files and create an options just to change them.

@igrigorik

igrigorik Feb 23, 2012

Owner

Can you elaborate on the *.js.erb part? Why is this a concern for closurebuilder? Seems like if the order is right then it should be sprockets doing the work to process .js.erb, etc, files. Which also comes back to previous note about "unregistering" the default processor. If we override the default, then we loose all the .js.{erb,coffee, etc}.

@egorkhmelev

egorkhmelev Feb 23, 2012

Contributor

This is a typical application steps:

  1. Closure needs start point for deps calculation thats why 'require_closure' directive was developed for.
  2. Closurebuilder searching within all the '.js' files (download treescan.py from closure svn to check, string 28) and trying to find any with 'goog.provide' directive
  3. Closurebuilder calculates proper order and then gem includes them all in right order to project by 'require_asset' function
  4. Sprockets handle any .js.{erb,coffee, etc} postprocessing.

As you can see closurebuilder don't care about non '.js' files (step 2), and sprockets cannot handle js.erb files until that files included to page by 'require_asset' (step 3). So closurebuilder should search for 'goog.provide' directive even in .js.{erb,coffee, etc}. That why we need to change default regexp in treescan.py

If you could find better way it would be great. I couldn't. As I told erlier, new processor is inherited from unregistered one just to provide 'require_closure' directive for step 1.

@igrigorik

igrigorik Feb 23, 2012

Owner

Right, we're mixing two dependency management systems here, which is dangerous territory.

Option (a): force the user to stick with closure. There is something to be said for consistency and not trying to mix the paradigms.
Option (b): allow Rails deps management to run prior to closure. Specifically, I think the simplest way to make this work is not try to mix closure and rails, but serialize them. The rails deps management should run first, which will output a concatenated file (which we can write to a tmp file or similar), and then point closurebuilder at that file and let it do its thing.

P.S. http://bolinfest.com/coffee/ - that actually points towards option (a), I think.

+ results = `#{cmd} --output_mode=list`.split "\n"
+
+ # Finally require calculated assets
+ results.each{|asset| context.require_asset asset };
@igrigorik

igrigorik Feb 23, 2012

Owner

Don't need the ;

-
- app.assets.register_preprocessor 'application/javascript', ClosureDependenciesProcessor
+ # For developing purposes, Sprockets used to cache a lot even when server restarted
+ # app.assets.cache = false
@igrigorik

igrigorik Feb 23, 2012

Owner

Config option?

@egorkhmelev

egorkhmelev Feb 23, 2012

Contributor

I just need that for myself. Yes, this is default option. I gonna remove this comment.

@@ -1,13 +1,12 @@
module ClosureProcessor
class Railtie < Rails::Engine
- config.closure = ActiveSupport::OrderedOptions.new
- config.closure.lib = 'vendor/assets/closure-library/closure'
@igrigorik

igrigorik Feb 23, 2012

Owner

Don't we still need the closure-library checkout to build the actual, resulting javascript?

@joe1chen

joe1chen Feb 23, 2012

Actually I'm wondering if there's a better way to structure the assets so that only the css files are included. If you include the entire closure directory, you get a bunch of crap files in your assets folder (test html files, csproj files, readme's, etc.).

You actually want to add closure/goog/css (and possibly closure/css/inlay). The js files aren't needed because we're appending them to application.js. You don't even need the images in closure/goog/images because the css files reference the online cdn versions of the images.

When I was manually integrating closure, I just copied closure/googl/css to my vendor/assets.

@igrigorik

igrigorik Feb 23, 2012

Owner

Right, well hence the option to make the lib directory configurable. Some people like to freeze the checkout (ex, team environments), whereas you may want to maintain a checkout somewhere else. I don't think we should be removing this option.

@egorkhmelev

egorkhmelev Feb 23, 2012

Contributor

We don't need options to point on closure library anymore. Because calcdeps get all assets paths and looking there for closure library and dependencies. So user need to place closure library anywhere and properly setup assets.

@igrigorik

igrigorik Feb 23, 2012

Owner

Ah, gotcha - thanks.

Owner

igrigorik commented Feb 23, 2012

@lypanov fair enough. I'm not a fan of forcing the python dependency, but given that the current version is effectively broken without it.. that's a strong argument to merge this in.

@hmelyoff can you please remove the .pyc files?

+ # For developing purposes, Sprockets used to cache a lot even when server restarted
+ # app.assets.cache = false
+
+ app.assets.unregister_preprocessor 'application/javascript', Sprockets::DirectiveProcessor
@igrigorik

igrigorik Feb 23, 2012

Owner

Is there any specific reason to disable the default sprockets processor? In principle, you could mix and match default Rails logic with closure - although, not sure if I would recommend this. :)

@egorkhmelev

egorkhmelev Feb 23, 2012

Contributor

New processor that registered insted of default inherited from default. I've just add new directive "require_closure".

@igrigorik

igrigorik Feb 23, 2012

Owner

Ah, interesting - didn't catch that.

lypanov commented Feb 23, 2012

I should have mentioned that I was using the modern builder.

IMO the namespace stuff is pretty fundamental to closure and it's only a one liner so while yes, it breaks the blog post, it enables code reuse which I suspect the existing code does not allow for. Allowing includes becomes important very fast.

Owner

igrigorik commented Feb 23, 2012

I wouldn't worry about breaking the blog post - that should be the least of our concerns! The blog post can be updated at any time :)

xrd commented Mar 20, 2012

@igrigorik, @egorkhmelev, @lypanov thank you for working on this issue, and @igrigorik thank you so much for this gem.

Is this pull request dead? I'm trying to use closure-sprockets and cannot get past the example on the front page. When I try to add this line to my test.js file:

goog.require('goog.ui.Button');

I get a crash when generating the list of JS files:

couldn't find file 'goog/array/arraylike' (in /Users/xrdawson/Projects/closure-rails/vendor/assets/closure-library/closure/goog/ui/component.js)

I've tried using @egorkhmelev's fork with this pull request, but then I get this issue:

NameError: uninitialized constant Sprockets::DirectiveProcessor ~/.rvm/gems/ruby-1.9.2-p290/bundler/gems/closure-sprockets-f9cdd0633736/lib/closure-sprockets/directive_processor.rb:1:in<top (required)>'
~/.rvm/gems/ruby-1.9.2-p290/bundler/gems/closure-sprockets-f9cdd0633736/lib/closure-sprockets.rb:4:in require' ...

I'm using Rails 3.2.2. I've tried using a gemset for Rails 3.1 and I get the same crash with @igrigorik's version, and "goog is not defined" from @egorkhmelev's (it does not build the closure dependency JS list at all.)

When I use @egorkhmelev's version, I do see Python being spun up in the process list, so I assume it is using the newer dependency build script.

Does anyone have a basic rails project which I can use to start using closure library inside of rails?

Thanks.
Chris

Owner

igrigorik commented Mar 22, 2012

I'd love to get this merged. There are a few outstanding issues in the discussion above that need to resolved first. @lypanov is this something you have bandwidth to make changes for?

lypanov commented Mar 22, 2012

I'm actually using clojurescript now via clementine and no longer use closure-sprockets so alas not sorry! Maybe @egorkhmelev has time to push it on through :)

xrd commented Mar 22, 2012

I will review the changed and see if I can take up from where things got
left off. I am wondering if this is because I am using coffee script rather
than just straight js. Thanks again everyone.

Chris Dawson
Human potential, travel and connection
http://webiphany.com
On Mar 22, 2012 4:05 AM, "Alexander Kellett" <
reply@reply.github.com>
wrote:

I'm actually using clojurescript now via clementine and no longer use
closure-sprockets so alas not sorry! Maybe @egorkhmelev has time to push it
on through :)


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

Collaborator

guilleiguaran commented Apr 24, 2012

@igrigorik I will be checking this later this week

BertHartm and others added some commits Jul 25, 2012

Merge pull request #1 from BertHartm/master
Clear up Uninitialized symbol Sprockets::DirectiveProcessor error
Owner

igrigorik commented Sep 28, 2012

@BertHartm @guilleiguaran sorry, dropped the ball.. where are we with this one?

Fork by @egorkhmelev is working beautifully for me (https://github.com/SpainTrain/mynotes).

@igrigorik Is the main outstanding issue whether to use calcdeps or builder (or make it a config option)? I'd be willing to polish off what is left.

Owner

igrigorik commented Sep 29, 2012

@SpainTrain to be honest, I've lost the thread myself as well - it's not clear what is missing. If you can confirm that what we have here works, then we can merge. If not, and you're willing to pull it over the line.. then you're officially awesome. :-)

@igrigorik my app doesn't use some features (like soy). I'll make up a test app to confirm everything is good. Hah, I officially just needed to get my app faster! Definitely glad this was available.

Contributor

BertHartm commented Sep 30, 2012

@igrigorik Everything works great for me on @egorkhmelev 's fork (after the patch). speed is an issue, but not enough of one to make me care.

Owner

igrigorik commented Sep 30, 2012

@egorkhmelev can you make a pull? Happy to merge it in.

Contributor

egorkhmelev commented Oct 12, 2012

I have made a pull 3 months ago, so you could merge

Owner

igrigorik commented Oct 12, 2012

Ah, well, my bad then! Will merge in a sec.

igrigorik added a commit that referenced this pull request Oct 12, 2012

Merge pull request #10 from egorkhmelev/master
Fixed version of gem that calculate dependensies in proper way

@igrigorik igrigorik merged commit 43e9423 into igrigorik:master Oct 12, 2012

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