cdnify ignores paths in base URL

billpatrianakos commented 9 years ago

I'm having an issue where if my CDN URL contains a path like in the example (/stuff/) the URLs in the generated files do have the CDN URL prepended to them but they're missing the path. This is happening on images using root-relative paths.

My config looks like this:

cdnify: {
      dist: {
        options: {
          base: 'http://my.cdn.com/marketing/'
        },
        files: [{
          expand: true,
          cwd: '<%= app.dist %>',
          src: '**/*.{css,html,js}',
          dest: '<%= app.dist %>'
        }]
      }
    }

The generated URLs end up as http://my.cdn.com/img/whatever, missing the /marketing part. I made sure there were no other tasks doing work before or after CDNify that would interfere.

Right now I only notice it on images because they're the only thing referencing external assets in my project (I'm building a bunch of HTML emails).

Any ideas what could be wrong? For now my workaround is to use the rewriter function and match to any path where indexOf('/img') === 0 but that won't work when assets aren't root-relative and its more than just images to worry about.

Climax777 commented 9 years ago

This is a major problem for us +1

kirkhoff commented 9 years ago

@Climax777 and @billpatrianakos I created a PR for the fix. It's linked above.

billpatrianakos commented 9 years ago

That's awesome. Thanks, @kirkhoff I think I might start using your fork until its officially merged. Thanks again!

snowbillr commented 9 years ago

:+1: on this issue for us, we need this for our deployment

XhmikosR commented 9 years ago

Does this still happen with the latest master?

billpatrianakos commented 9 years ago

I've not used it since reporting the issue. I haven't seen any word on an official fix yet. I guess the best way to find out is to try and see.

XhmikosR commented 9 years ago

We switched to ulr.resolve for the paths that is why I ask.

kirkhoff commented 9 years ago

I don't believe the fix I put in was ever merged. @callumlocke any chance we could have this added? I referenced my PR above.

XhmikosR commented 9 years ago

Well the url fix is added.

Otherwise make clear pull requests with one feature per change and CC me.

Also make sure you add a test for the failing cases. On Oct 19, 2015 16:38, "Curt" notifications@github.com wrote:

I don't believe the fix I put in was ever merged. @callumlocke https://github.com/callumlocke any chance we could have this added? I referenced my PR above.

— Reply to this email directly or view it on GitHub https://github.com/callumlocke/grunt-cdnify/issues/18#issuecomment-149215621 .

kirkhoff commented 9 years ago

@XhmikosR this is what I was referring to. If it's fixed, then :+1:

infomofo commented 8 years ago

This does not seem to be fixed on version 1.0.2

albogdano commented 8 years ago

~~Not fixed! It worked fine in v0.1.1 then it got broken and it's the reason why I'm still on 0.1.1.~~

This actually works as expected, my CDN url in the base option was missing a slash at the end and that failed to resolve properly.

XhmikosR commented 8 years ago

@albogdano: Then we'll need a clear test case.

albogdano commented 8 years ago

Ok, so I had the same problem but after investigating the issue it turns out the behavior of url.resolve() is correct and as expected. The problem people have comes from the fact the their relative URLs begin with /, for example:

<a href="/img/whatever/icon.png">

points to the root folder so it becomes

<a href="http://my.cdn.com/img/whatever/icon.png">

even if you set CDN URL to be http://my.cdn.com/marketing/.

The solution is to either remove the slash from the beginning of your relative URLs, or have an option that tells this module to remove these slashes for you. I think this should be closed.

callumlocke / grunt-cdnify

cdnify ignores paths in base URL #18