Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace paths doesn't handle substrings correctly, and only replaces first instance of a url #22

Closed
MalucoMarinero opened this issue Oct 15, 2018 · 2 comments

Comments

@MalucoMarinero
Copy link
Contributor

MalucoMarinero commented Oct 15, 2018

Hey there, I find staticify real handy in a prebuild and deploy scenario, there's just a few issues with replacePaths that mean it doesn't currently work completely for my use case:

  1. Substrings are often incorrectly replaced
@font-face {
  font-family: 'Portrait Web';
  src: url('/assets/font-portrait/Portrait-Regular-Web.eot');
  src: url('/assets/font-portrait/Portrait-Regular-Web.eot?#iefix') format('embedded-opentype'),
       url('/assets/font-portrait/Portrait-Regular-Web.woff2') format('woff2'),
       url('/assets/font-portrait/Portrait-Regular-Web.woff') format('woff');
  font-weight:  400;
  font-style:   normal;
  font-stretch: normal;
}

Consider CSS that looks like this, if Object.keys returns the woff path before the woff2 path, the hash for the .woff path will actually end up on the .woff2, meaning a broken link in production.

Should be easily fixed with a length sort of Object.keys(versions), so longer strings get handled before shorter strings.

  1. String.replace only replaces the first instance when used with strings.
    Usually not an issue in my builds, but if a file is referenced more than once replacePaths won't currently pick it up. Better to turn it into a new RegExp(url, "g"), the global flag will ensure it gets everything.

I think this code should fit the bill but I didn't want to create a pull request and be presumptive, let me know if you want me to create one.

    const replacePaths = fileContents => {
        return Object.keys(versions).sort(function(a, b) { return b.length - a.length })).reduce((f, url) => {
            return f.replace(new RegExp(url, "g"), getVersionedPath(url));
        }, fileContents);
    };

Cheers!

@XhmikosR
Copy link
Collaborator

You are welcome to make a PR with the fix and tests 👍

MalucoMarinero added a commit to weareoffsider/staticify that referenced this issue Dec 9, 2018
…n replacePaths

Sort strings so longer strings are handled first, so the longer
paths aren't mistakenly given a hash of some shorter substring path.

Add global flag to replacePaths regex to ensure all instances of a path
are handled.
@MalucoMarinero
Copy link
Contributor Author

Done, I'll leave it with you. Passes xo and mocha test.

XhmikosR pushed a commit that referenced this issue Feb 28, 2019
…hs (#25)

Sort strings so longer strings are handled first, so the longer
paths aren't mistakenly given a hash of some shorter substring path.

Add global flag to replacePaths regex to ensure all instances of a path
are handled.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants