christophercliff / metalsmith-fingerprint

A fingerprint plugin for Metalsmith
MIT License
28 stars 7 forks source link

normalize file paths #8

Closed oupala closed 9 years ago

oupala commented 9 years ago

normalize file path so that the plugin works on linux and windows (and hopefully macos X)

just a try to fix issue #7

christophercliff commented 9 years ago

Does path.normalize(p) fix this?

EDIT: fixed link.

oupala commented 9 years ago

Nope, path.normalize(p) does not fix this.

Let me explain it as clearly as I can.

This is what I have in my html :

<link href="/{{fingerprint.[css/app.css]}}" rel="stylesheet" type="text/css" />

This is what I have in metalsmith' metadata :

css\app.css (I think metalsmith automatically normalize the path on windows)

This is how it is modified by path.normalize(p) :

css\app.css (yes, it is not modified, but this is normal)

This is what is done by my pull request :

css/app.css (this is what I want as on the web, we use slashes and not backslashes)

In fact, path.normalize(p) is meant to normalize the path to the local system. It converts \ to / when running on linux and / to \ when running on windows (among other things.

In this case, I want to generate de linux path on a windows system, because the generated path will be written in the html file and the web is using / as common delimiter.

This is why I can't use path.normalize(p) in my case.

This is also why I created the unixp variable so that we can have the real path of the css file p (with / on linux and \ on windows) and the path that will be written to the html file (always with / whatever the system metalsmith is running on).

My pull request is probably not optimal and can be improved, but it works as is.

I hope my explanations were clear enough.

oupala commented 9 years ago

@christophercliff Have you seen this pull request?

christophercliff commented 9 years ago

I'm not sure we need https://github.com/oupala/metalsmith-fingerprint/blob/oupala-normalize/lib/index.js#L18. Could you please change https://github.com/oupala/metalsmith-fingerprint/blob/oupala-normalize/lib/index.js#L21 to read:

var fingerprint = [
    p.substring(0, p.lastIndexOf(ext)),
    '-',
    hash,
    ext,
].join('').replace(/\\/g, '/')

I haven't tested, but if that's valid I'll merge it.

oupala commented 9 years ago

I adopted the solution you suggested, I tested it, and it is working.

Thanks for this improvement to my pull request @christophercliff !