christophercliff / metalsmith-fingerprint

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

fingerprinted object path should be normalized #7

Closed oupala closed 7 years ago

oupala commented 9 years ago

I'm trying to use this plugin and I'm close to succeed.

create a duplicate of index.css with a fingerprinted filename

This works well.

create a fingerprint object on the Metalsmith metadata

This works almost well. But it seems that the created object in metalsmith has no normalized path. As a consequence, the way of accessing the object is different depending of if I'm on linux or on windows :

This makes impossible to use this plugin when developing on windows and integrating on linux.

Normalizing the path should make the plugin OS agnostic.

oupala commented 9 years ago

My pull request is just a try but at least it works on my windows system. It might be quick and dirty, but feel free to improve it.

And I have absolutely no idea how to test this, unfortunately I won't be able to add tests to this pull request. I'm sorry for this but I've done my best.

CumpsD commented 9 years ago

I would very much love to see this PR merged and a new version pushed, I am developing on both Windows and Linux and that's giving a lot of problems.

CumpsD commented 9 years ago

In the mean time, you can always do this:

var fixFingerprint = function () {
    'use strict';

    return function (files, metalsmith, done) {
        var metadata = metalsmith.metadata(),
            fingerprint  = metadata.fingerprint,
            newFingerprint = {};

        for (var key in fingerprint) {
            var newKey = key.replace(/\\/g, '/');
            var newValue = fingerprint[key].replace(/\\/g, '/');

            newFingerprint[newKey] = newValue;
        }

        metadata.fingerprint = newFingerprint;

        done();
    };
};

And use it:

        .use(fingerprint({ pattern: '{styles,scripts}/**/*.{css,js}' }))
        .use(fixFingerprint())

That will also give a cross platform working solution while waiting for a PR.

oupala commented 9 years ago

@christophercliff I'd be glad if you can merge my pull request.

christophercliff commented 9 years ago

Fixed in #8

flegris-orange commented 8 years ago

It appears that the issue is not fixed yet as it does not work on my windows machine, please reopen this issue.

I'm willing to test any potential solution on my env.

flegris-orange commented 8 years ago

8 was correctly fixing the problem as in this state: https://github.com/christophercliff/metalsmith-fingerprint/blob/b1f635a5e9d6de37ede1fdd3f450861b76de5768/lib/index.js

Tested and validated on Windows.

oupala commented 8 years ago

Ok, let's recap the commits and the pull requests trying to fix this issue.

There has been 4 commits and 2 pull requests.

  1. first pull request #8
    • first commit ce89aa916d8459ab97494f5bdf8f7405090d8d36 : was working, but needed some linting
    • second commit b1f635a5e9d6de37ede1fdd3f450861b76de5768 : was also working, linted, but @christophercliff wanted a much simpler solution
    • third commit e2fba8590a45d2591df6c457ec5148861c15d6d8 : was not working :-( but was merged
  2. second pull request #9
    • first commit 22e56406783dda815c57ecc08f65d40f2c2ba82a : was working, but not merged

As you can see the only merged pull request was merged at a state were it was not working.

There is still two commits that work and could be merged. The first one is the most elegant while the second one seems more simpler.

oupala commented 8 years ago

Hey @christophercliff, you added the pull request appreciated label.

There is a pull request.

If I could I would add the merge appreciated label!

flegris-orange commented 7 years ago

I would greatly appreciate to see any pull request merged. Any news about that?

oupala commented 7 years ago

I created a new pull request #13 so that you can merge it easily.