smysnk / gulp-rev-all

Static asset revisioning with dependency considerations, appends content hash to each filename (eg. unicorn.css => unicorn.098f6bcd.css), re-writes references.
https://www.npmjs.org/package/gulp-rev-all
MIT License
422 stars 83 forks source link

Issue with paths on windows #15

Closed tadeuszwojcik closed 10 years ago

tadeuszwojcik commented 10 years ago

When I use this module on Windows, everything works great, except paths use windows style slashes (backslashes) for example:

Found root reference [ app/file-picker/test.jpg ] -> [ app\filepicker\test.rev.0ca82d28161aa4a3171fc2ce.jpg ]

Is it related with using path.join? I'd be really grateful if you could fix it :)

smysnk commented 10 years ago

I do believe you are right it is related to path.join. Not sure graceful way to resolve this besides replacing '\' with '/' for each path.join.

smysnk commented 10 years ago

This should be fixed in 0.3.1, please give it a try.

tadeuszwojcik commented 10 years ago

Thanks!

jeffborden commented 10 years ago

I'm looking at the implementation of joinPath in 0.5.2 and while I do see the update to replace '\' w/ '/' it seems that it's only replacing the first instance of a backslash. It seems that the following:

var joinPath = function (directory, filename) {
    return path.join(directory, filename).replace('\\', '/');
};

should be replaced w/

var joinPath = function (directory, filename) {
    return path.join(directory, filename).replace(/\\/g, '/');
};
koistya commented 10 years ago

Still there are issues with paths on Windows :( for example revAll({ ignore: [ /^\/favicon.ico$/g ]}) ignore rules don't work.

smysnk commented 10 years ago

What are your ignore rules that are not working? Have you tried switching the slash style?

koistya commented 10 years ago

Switching the slash style would make it non-compatible with Unix systems. /^\/favicon.ico$/g this rule should work as it is on both Unix and Windows (consider a scenario where you develop on Windows but deploy from Linux, or develop on Mac but deploy from Windows machine)

smysnk commented 10 years ago

Yep, I agree it should. Just wondering if switching slash style will cause ignore to catch it? Will help me identify the problem.

koistya commented 10 years ago

Another issue is that it prints full paths to the log (though this one is not blocking issue):

[21:38:42] gulp-rev-all: Skipping binary file [ C:\Projects\Project1\build\favicon.ico ]

But it should be:

[21:38:42] gulp-rev-all: Skipping binary file [ favicon.ico ]

Probably a fix could look similar to this one: https://github.com/pgherveou/gulp-awspublish/pull/24

koistya commented 10 years ago

@smysnk Okay, checking... So far, changing /^\/favicon.ico$/g to /^\\favicon.ico$/g does not make any difference.

koistya commented 10 years ago

@smysnk Changing /^\/favicon.ico$/g to /^\favicon.ico$/g works fine on Windows.

smysnk commented 10 years ago

@koistya Okay cool, I'll check into it. Thanks!

smysnk commented 10 years ago

Should be fixed in ea7a6d11a131111953df003ffa894ca94aa81847

koistya commented 10 years ago

Testing these rules:

.pipe($.revAll({
    ignore: [
        /^\/apple-touch-icon-precomposed.png$/g,
        /^\/browserconfig.xml$/g,
        /^\/crossdomain.xml$/g,
        /^\/error.html$/g,
        /^\/favicon.ico$/g,
        /^\/humans.txt$/g,
        /^\/robots.txt$/g
    ]
}))
.pipe(awspublish.gzip())
.pipe(publisher.publish())

This what is printed to the log:

Root directory [ C:\Projects\koistya\tarkus.me\build\ ]
Skipping binary file [ apple-touch-icon-precomposed.png ]
Finding references in [ browserconfig.xml ]
Skipping binary file [ tile.png ]
Skipping binary file [ tile-wide.png ]
Found relative reference [ tile.png ] -> [ tile.b1903792.png ]
Found relative reference [ tile-wide.png ] -> [ tile-wide.5e88282b.png ]
Finding references in [ crossdomain.xml ]
Finding references in [ error.html ]
Skipping binary file [ favicon.ico ]
Finding references in [ humans.txt ]
Finding references in [ robots.txt ]
Skipping binary file [ tile-wide.png ]
Skipping binary file [ tile.png ]
Root directory [ C:\Projects\koistya\tarkus.me\build\ ]
[create] browserconfig.b1c6ec30.xml
[create] crossdomain.78a0aca5.xml
[create] error.5988ca8a.html
[create] favicon.8f384bfb.ico
[create] humans.906f2000.txt
[create] robots.8e8ac4eb.txt
[create] tile-wide.5e88282b.png
[create] tile.b1903792.png

gulp-awspublish still pushes renamed files to Amazon S3.

Sample project: https://github.com/koistya/tarkus.me

smysnk commented 10 years ago

Give c1fa0dafcf95cdb4ee874458a9d675d9ae5f6f29 a try, think should be finally fixed now. It is suppose to detect the root directory without a trailing slash.

koistya commented 10 years ago
gulp-rev-all: Root directory [ C:\Projects\koistya\tarkus.me\build\ ]
gulp-rev-all: Skipping binary file [ apple-touch-icon-precomposed.png ]
gulp-rev-all: Finding references in [ browserconfig.xml ]
gulp-rev-all: Skipping binary file [ tile.png ]
gulp-rev-all: Skipping binary file [ tile-wide.png ]
gulp-rev-all: Found relative reference [ tile.png ] -> [ tile.b1903792.png ]
gulp-rev-all: Found relative reference [ tile-wide.png ] -> [ tile-wide.5e88282b.png ]
gulp-rev-all: Finding references in [ crossdomain.xml ]
gulp-rev-all: Finding references in [ error.html ]
gulp-rev-all: Skipping binary file [ favicon.ico ]
gulp-rev-all: Finding references in [ humans.txt ]
gulp-rev-all: Finding references in [ robots.txt ]
gulp-rev-all: Skipping binary file [ tile-wide.png ]
gulp-rev-all: Skipping binary file [ tile.png ]
[skip]   apple-touch-icon-precomposed.4f2ddf11.png
gulp-cloudfront: Root directory [ C:\Projects\koistya\tarkus.me\build\ ]
[skip]   browserconfig.b1c6ec30.xml
[skip]   crossdomain.78a0aca5.xml
[skip]   error.5988ca8a.html
[skip]   favicon.8f384bfb.ico
[skip]   humans.906f2000.txt
[skip]   robots.8e8ac4eb.txt
[skip]   tile-wide.5e88282b.png
[skip]   tile.b1903792.png
koistya commented 10 years ago

@smysnk /[\//]$/ --> /[\\/]$/

smysnk commented 10 years ago

Not sure how I messed that up lol. Anyways added some unit tests around it too so hopefully it really is fixed this time.