Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Adding CSS `url()` rewriting #32

Open
wants to merge 4 commits into from

4 participants

@pferreir

Hi there!

I was having some problems with url() statements in minified/bundled CSS files containing relative URLs. So, I added URL rewriting capability to Bundle up. I've played with it a bit and it seems to work just fine. The unit tests seem to be OK too.

I've also added a generatedDir config option, just because it seemed right :) I hope it's OK with you.

Could we have this in the official release? I tried to carefully check my code since I'm quite new to CoffeeScript. So, feel free to correct it.

Thanks!

@pferreir

There's a unit test failing, but I believe it's got nothing to do with my changes. It's failing in master too.

@Cowboy-coder

Awesome, thanks a lot! :) I will try to get this merged in the a day or two. I will also fix the failing test. Would it be possible for you to add some unit tests for this CSS URL Rewriting thing? Not a biggie, I can probably fix it during merge-time otherwise but it helps me understand this feature as well. Is it rewriting

background: url(/images/bg.jpg)

to

/* assuming staticUrlRoot:"http://static.example.com" */
background: url(http://static.example.com/images/bg.jpg)

?

@pferreir

No, it is rather comparing the initial directory the CSS file was in with the final "bundle" one, and prepending (and normalizing) the corresponding relative path before each url(...) resource.
I will add some unit tests later today.

@Cowboy-coder

Cool! I fixed the failing test in master btw.

@pferreir

OK, I've made a small fix in a regular expression.
I've also added two small unit tests. I hope they're OK. Please let me know if that's not the case :)

@Cowboy-coder

Looked at this patch yesterday and it looked great. I just need to figure out why I get an 'Unexpected Indent¨-error when I try bundle-up from the current Master branch (not noticed when I run tests). Probably some changes in coffee-script that's causing the error. Will merge this pull request after I've fixed that.

@hammat

Hi guys, thanks for your great work!
Any update about merging pferreir's pull request?
Cheers

@Cowboy-coder

Sorry guys for the late response.

This pull request seems to be breaking some styles for me url(/relative/to/root.png) gets rewritten to url(../relative/to/root.png) I think it should only replace urls if the path doesn't start with a /. Also it breaks base64 encoded images.

But do we really need this? Wouldn't it be easier to just reference the images starting with a /, which would solve all these issues, because then the images would be relative to the web server root instead of relative to the stylesheet?

@danielsantiago

@Cowboy-coder but what about some external libraries like "font-awesome" that use relative path by default to find font files. I thinks this is a must.

@pferreir

I agree with @hlwebs, having relative URLs is kind of a must. I, for one, tend to avoid using absolute URLs in CSS files since I like my apps to be deployable everywhere, even in http://myserver/myapp/ setups.

I believe that just excluding paths starting with / would suffice. I can have a look at it later, if you agree that it's the way to go.

@danielsantiago

@pferreir have you ever look at Assetic (asset management framework for PHP)? They have a css rewrite filter that fixes relative URLs, maybe we can follow they implementation: https://github.com/kriswallsmith/assetic/blob/master/src/Assetic/Filter/CssRewriteFilter.php

I have never done anything in CoffeeScript but I will try to help.

@Cowboy-coder

Yeah, ok, I agree! :)

Would actually if possible be pretty cool if this was based on staticUrlRoot in some way. Like if I send staticUrlRoot:"http://cdn-example.com" all urls (relative and absolute) in the css would properly be prefixed with "http://cdn-example.com".

What do you guys think?

@pferreir

Even URLs that start with / ? I would expect those to refer server/, regardless of what staticUrlRoot says...
I think there are two different use cases here:

  1. Fixing relative URLs that get broken because the final path of a bundle is different from its "development" one;
  2. Replacing or re-locating relative or root (/) URLs;

I was trying to address the first case, but maybe with some extra work we can kill the two birds with one stone.
For instance, webassets uses an interesting approach:

https://github.com/miracle2k/webassets/blob/master/src/webassets/filter/cssrewrite/__init__.py#L78

Basically, the filter accepts a "replace" function or a dictionary mapping paths, otherwise it falls back to doing point 1 above.

@danielsantiago

I really like the idea of a "replace function" for a custom need.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 11, 2013
  1. @pferreir
Commits on Apr 14, 2013
  1. @pferreir
  2. @pferreir
Commits on Jul 1, 2013
  1. @pferreir
This page is out of date. Refresh to see the latest.
View
15 lib/bundle.coffee
@@ -7,6 +7,8 @@ fs = require 'fs'
class Bundle
constructor: (@options) ->
@options.staticRoot = path.normalize(@options.staticRoot)
+ @options.generatedDir = options.generatedDir || '/generated/'
+
@files = []
@defaultNamespace = 'global'
@@ -33,6 +35,9 @@ class Bundle
fileExt = fileExt[fileExt.length - 1]
return fileExt != 'js' and fileExt != 'css'
+ _needsUrlRewrite: ->
+ false
+
addFilesBasedOnFilter: (filterPath, namespace) ->
directoryPath = filterPath.substring(0, filterPath.indexOf('*'))
@@ -97,8 +102,9 @@ class Bundle
# Determine if we need to copy/compile
# the file into the staticRoot folder
+
if (file.indexOf(@options.staticRoot) == -1 or @_needsCompiling(file))
- writeTo = path.normalize(@_convertFilename(@options.staticRoot + '/generated/' + relativeFile))
+ writeTo = path.normalize(@_convertFilename(@options.staticRoot + @options.generatedDir + relativeFile))
needsCompiling = true
file = writeTo
relativeFile = @_getRelativePath(file)
@@ -122,8 +128,13 @@ class Bundle
str += fs.readFileSync(file.file, 'utf-8').trim('\n') + '\n'
str = @minify(str)
+
+ if @_needsUrlRewrite()
+ relUrl = path.relative(@options.staticUrlRoot + @options.generatedDir + 'bundle', path.dirname file.url)
+ str = @_rewriteUrls(str, relUrl)
+
hash = crypto.createHash('md5').update(str).digest('hex')
- filepath = "#{@options.staticRoot}/generated/bundle/#{hash.substring(0, 7)}_#{namespace}#{@fileExtension}"
+ filepath = "#{@options.staticRoot}" + @options.generatedDir + "bundle/#{hash.substring(0, 7)}_#{namespace}#{@fileExtension}"
writeToFile(filepath, str)
View
11 lib/css.coffee
@@ -1,13 +1,22 @@
csso = require 'csso'
Bundle = require './bundle'
+path = require 'path'
+
class Css extends Bundle
constructor: (@options) ->
@fileExtension = '.css'
super
+ _rewriteUrls: (code, relativeURL) ->
+ code.replace /url\(['"]?([^\)"'\/][^\)"']*)['"]?\)/g, (match, $1) ->
+ resUrl = path.normalize(path.join(relativeURL, $1))
+ return "url('#{resUrl}')"
+
+ _needsUrlRewrite: ->
+ @options.bundle == true
+
minify: (code) ->
return code unless @options.minifyCss
-
csso.justDoIt(code)
render: (namespace) ->
View
39 test/bundle_spec.coffee
@@ -65,3 +65,42 @@ describe 'bundle:true', ->
fs.stat @bundle.js.files[0].file, (err, stats) ->
expect(err).to.equal(null)
done()
+
+
+describe 'CSS URL Rewriting', ->
+ beforeEach ->
+ helper.beforeEach()
+ @app = express.createServer()
+
+ # Bundle once without minifying and then once _with_ minifying:
+
+ @bundle = BundleUp @app, __dirname + "/files/relative.coffee",
+ staticRoot: __dirname + "/files/public/",
+ staticUrlRoot: "/",
+ bundle: true,
+ minifyJs: false,
+ minifyCss: true
+
+ @bundle_no = BundleUp @app, __dirname + "/files/relative.coffee",
+ staticRoot: __dirname + "/files/public/",
+ staticUrlRoot: "/",
+ bundle: false,
+ minifyJs: false,
+ minifyCss: false
+
+ it 'Bundled CSS should have their URLs rewritten (except for the ones starting with `/`)', (done) ->
+ fs.readFile @bundle.css.files[0].file, (err, data) ->
+ data = data.toString()
+ expect(data.match(/'\.\.\/css\/relative\/path\/to\/file1\.jpg'/).length).to.equal(1)
+ expect(data.match(/'\.\.\/css\/relative\/path\/to\/file2\.jpg'/).length).to.equal(1)
+ expect(data.match(/'\.\.\/css\/relative\/path\/to\/file3\.jpg'/).length).to.equal(1)
+ expect(data.match(/'\/absolute\/path\/to\/file4\.jpg'/).length).to.equal(1)
+ done()
+
+ it 'Non-bundled CSS should have their URLs intact', (done) ->
+ fs.readFile @bundle_no.css.files[0].file, (err, data) ->
+ data = data.toString()
+ expect(data.match(/'relative\/path\/to\/file1\.jpg'/).length).to.equal(1)
+ expect(data.match(/"relative\/path\/to\/file2\.jpg"/).length).to.equal(1)
+ expect(data.match(/relative\/path\/to\/file3\.jpg/).length).to.equal(1)
+ done()
View
15 test/files/css/relative.css
@@ -0,0 +1,15 @@
+body {
+ background: url('relative/path/to/file1.jpg');
+}
+
+div#main {
+ background: url("relative/path/to/file2.jpg");
+}
+
+div.menu {
+ background: url(relative/path/to/file3.jpg);
+}
+
+div.menu2 {
+ background: url('/absolute/path/to/file4.jpg');
+}
View
3  test/files/relative.coffee
@@ -0,0 +1,3 @@
+module.exports = (assets) ->
+ assets.root = __dirname
+ assets.addCss('/css/relative.css')
Something went wrong with that request. Please try again.