Js include tag fix #6729

Merged
merged 0 commits into from Jan 4, 2013

Projects

None yet

5 participants

@noahsilas

In a production environment where the assets have been precompiled, we
don't want an assets compile step to happen on the application server at
all. To ensure this, a js runtime may not be available on the app
servers. In this environment, pages using javascript_include_tag for
assets with non-standard or chained extensions were throwing 500 errors.
For instance, javascript_include_tag('jquery.min') would blow up.

Sprockets was attempting to build the assets being included during the
rewrite_extension step (responsible for appending a '.js' extension to
assets being included by the basename rather than a fully qualified
name). This was happening as a step to resolve #6310, which required
checking for the presence of an asset with a non-standard extension
before appending the extension.

We can check for the presence of an asset without invoking the asset
build step by using Sprockets' resolve method, which will search for the
base file without building it (and is the method that find_asset uses
internally to get the path to the asset before attempting to build it).

When rewriting the extension on an asset, these are the steps:

  • If the source does not have an extension, assume that the default
    extension is desired and append it.
  • If there is an extension and it doesn't match the default extension,
    check to see if a file with the precise name specified exists amongst
    the assets; if it is present, do not append the default extension.
    (This is the step that resolves #6310).
@steveklabnik
Member

@guilleiguaran , what do you think about this?

@guilleiguaran
Member

This seems fine for me but I would like the opinion of others before of merge it.

/cc @spastorino @rafaelfranca @josevalim

@rafaelfranca
Member

It is fine to me too.

@guilleiguaran
Member

@noah256 also send a PR to add this to sprockets-rails

@rafaelfranca rafaelfranca commented on an outdated diff Sep 17, 2012
actionpack/test/template/sprockets_helper_test.rb
@@ -268,6 +268,14 @@ def compute_host(source, request, options = {})
javascript_include_tag(:application)
end
+ test "precompiled assets with an extension when no JS runtime is available" do
+ @config.assets.compile = false
+ @config.assets.digests = {'foo.min.js' => 'foo.min-f00.js'}
+ Sprockets::Index.any_instance.stubs(:build_asset).raises
+ assert_nothing_raised { javascript_include_tag('foo.min') }
+ end
+
+
@rafaelfranca
rafaelfranca Sep 17, 2012 Ruby on Rails member

Remove this line

@rafaelfranca rafaelfranca and 2 others commented on an outdated diff Sep 17, 2012
actionpack/lib/sprockets/helpers/rails_helper.rb
@@ -155,16 +155,24 @@ def rewrite_asset_path(source, dir, options = {})
end
def rewrite_extension(source, dir, ext)
@rafaelfranca
rafaelfranca Sep 17, 2012 Ruby on Rails member

What you think about this refactoring?

def rewrite_extension(source, dir, ext)
  source_ext = File.extname(source)[1..-1]

  if !ext || ext == source_ext
    source
  elsif source_ext.blank?
    "#{source}.#{ext}"
  else
    find_in_sprockets(source, ext)
  end
end

def find_in_sprockets(source, ext)
  pathname = asset_environment.resolve(source)

  if pathname.to_s =~ /#{Regexp.escape(source)}\Z/
    source
  else
    "#{source}.#{ext}"
  end
rescue Sprockets::FileNotFound
  "#{source}.#{ext}"
end

Maybe you can find a better name to find_in_sprockets method.

@guilleiguaran
guilleiguaran Sep 17, 2012 Ruby on Rails member

I like this since with this PR the rewrite_extension now looks so complex

@guilleiguaran
guilleiguaran Sep 17, 2012 Ruby on Rails member

Agree also with your comment, we need to find a better name for find_in_sprockets method

@steveklabnik
steveklabnik Sep 17, 2012 Ruby on Rails member

resolve_pathname?

@guilleiguaran guilleiguaran was assigned Oct 2, 2012
@steveklabnik
Member

@noah256 can you remove the extra line that @rafaelfranca asked for so @guilleiguaran can merge this in?

@rafaelfranca
Member

We need to check if this is still valid.

@brigade-gerrit brigade-gerrit merged commit 6b9cb71 into rails:3-2-stable Jan 4, 2013

1 check passed

Details default The Travis build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment