Fixed assets precompile regex #2876

Merged
merged 1 commit into from Sep 10, 2011

Projects

None yet

5 participants

@ay
ay commented Sep 5, 2011

The original regex was matching things like jquery.min.js and putting a lot of junk (like jquery-#{digest}.min.js) in public/assets/ that shouldn't be there. The new regex matches everything except files ending with .js or .css (as was intended).

@guilleiguaran
Ruby on Rails member

@alex3 can you provide a test case for this?

@spastorino
Ruby on Rails member

/cc @josh

@rhulse

This pull request is related to bug #1913

The orignal regex appears to want to include files that are not .js or .css files, but as you say is too broad.

@ay
ay commented Sep 6, 2011

@guilleiguaran I just wrote and pushed a test case for this. The commits are squashed into one.

@rhulse

Can I suggest that you add sanity tests for images and the other file types (including with multiple dots) that this is supposed to capture?

@ay
ay commented Sep 6, 2011

@rhulse I think the other tests test for images. Maybe an image named something like happy.face.png?

Also, should I push a new commit or squash it into one again?

@rhulse

I would expect so.

I suggested the extra tests because I spent some time (unsuccessfully) trying to fix that regex! I wonder if it's worth covering off underscores and dashes too. If this breaks there will be a lot of head-scratching and gnashing of teeth. :-)

@spastorino
Ruby on Rails member

@alex3 +1 to rhulse suggestion, please add those tests and I will merge the pull request

@rhulse

suggestions:

  • happy-face.png
  • happy_face.png
  • happy.happy_face.png
  • happy_happy.face.png
  • happy.happy.face.png

And NOT match:

  • .happy.face
  • .anything-with-a-dot-start

:-) !

@ay
ay commented Sep 6, 2011

@rhulse @spastorino All that in one test or a couple of them?

@spastorino
Ruby on Rails member

@alex3 as you find it better, perhaps you can iterate over a bunch of valid / invalid file names checking if they have been precompiled or not

@rhulse

Would it be a appropriate to extract the creation of this array into its own method, which can then include more comprehensive documentation on the intent? It may avoid future issues if there is refactoring.

@ay
ay commented Sep 7, 2011

@spastorino I just pushed a new (squashed) commit with the updated regex and a pretty comprehensive test. I've included all of @rhulse's suggestions in the test.

@rhulse That would probably be better but I think that should be a separate pull request.

@josh josh and 3 others commented on an outdated diff Sep 7, 2011
railties/lib/rails/application/configuration.rb
@@ -37,7 +37,7 @@ module Rails
@assets = ActiveSupport::OrderedOptions.new
@assets.enabled = false
@assets.paths = []
- @assets.precompile = [ /\w+\.(?!js|css).+/, /application.(css|js)$/ ]
+ @assets.precompile = [ /^[^.].*(\.(?!js$|css$)[^.]+)+\.*$|^[^.](?:(?!\.).)*\.*$/, /application.(css|js)$/ ]
@josh
josh Sep 7, 2011

Gnarly!

Good luck maintaing that regexp, rails core :)

@spastorino
spastorino Sep 7, 2011

lol, good luck to you too since you're part of the core :P

@rhulse
rhulse Sep 7, 2011

Yeah, use a regex and now you have two problems. ;-)

@ay
ay Sep 7, 2011

Perhaps I should split that regexp into two, and put each regexp on its own line with a comment explaining what it does?

@rhulse
rhulse Sep 7, 2011

I think putting this in a method on its own would 'minimize' the issue. If maintenance is a concern it could be more verbose in compiling the array.

@josh
Ruby on Rails member

Protip: This logic is now apart of rails itself. You can hack it.

https://github.com/rails/rails/blob/master/actionpack/lib/sprockets/assets.rake#L27-31

Maybe allow precompile to accept a Proc or something so you can do File.extname(path) != '.js'.

@ay
ay commented Sep 8, 2011

@josh See most recent commits.

@ay
ay commented Sep 8, 2011

@guilleiguaran Thanks, just pushed a new commit with your suggestion.

@josh josh and 2 others commented on an outdated diff Sep 8, 2011
railties/lib/rails/application/configuration.rb
@@ -37,7 +37,8 @@ module Rails
@assets = ActiveSupport::OrderedOptions.new
@assets.enabled = false
@assets.paths = []
- @assets.precompile = [ /\w+\.(?!js|css).+/, /application.(css|js)$/ ]
+ @assets.precompile = [ Proc.new{ |path| !File.extname(path).in?(['.js', '.css']) && path[0] != '.' },
@josh
josh Sep 8, 2011

Curious why != '.' is needed?

@ay
ay Sep 8, 2011

To ignore dot files like .gitkeep or something

@josh
josh Sep 8, 2011

Dot files should already be ignored by sprockets.

@ay
ay Sep 8, 2011

Oh, didn't know that... should I remove the path[0] != '.'?

@ay
ay Sep 8, 2011

All right, I'll have to change the test too.

@ay
ay commented Sep 8, 2011

Let me know if this is ready for merge and you want me to squash the commits.

@spastorino
Ruby on Rails member

I think this is ready to go, please rebase and squash the commits

@ay
ay commented Sep 8, 2011

@spastorino squashed

@spastorino
Ruby on Rails member

@alex3 the pull request cannot be merged, can you rebase it on top of the current master?

@spastorino spastorino merged commit be24be6 into rails:master Sep 10, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment