jamesknelson / gulp-rev-replace

Rewrite occurences of filenames which have been renamed by gulp-rev
MIT License
388 stars 62 forks source link

This fixes issue #1 #2

Closed sesponda closed 10 years ago

sesponda commented 10 years ago

Issue: if running under windows, the search pattern (e.g. "css\min.css") will not match text to be replaced (e.g."css/min.css"). This fix assumes that no one will be using windows-style patterns on the URLs.

sesponda commented 10 years ago

I'm just starting with NodeJS / Gulp, so feel free to ignore the pull request if the solution is not the recommended one.

Cryszon commented 10 years ago

You might want to use path.sep instead and make the regex global so that it matches more than the first \.

There's also another unrelated issue where specifying . as the base path loses the file extension, because the base replacement doesn't match from the beginning only. Here's a quick, dirty and completely untested fix for both of them. I'm too lazy to write an issue or a pull request for it right now, but here's the code anyway:

function relPath(base, filePath) {
  var newPath = filePath;

  // We should only strip the base if it's an actual base. The conditional
  // is used instead of regex, since we're dealing with non-regex-friendly
  // characters, such as "." or "/" that would need to be escaped.
  if(filePath.indexOf(base) === 0) {
      newPath.replace(base, '');
  }

  var sepReplace = new RegExp('\\' + path.sep, 'g');
  newPath = newPath.replace(sepReplace,'/');

  if (filePath !== newPath && newPath[0] === path.sep) {
    return newPath.substr(1);
  } else {
    return newPath;
  }
}

I'm not sure whether the base is supposed to be matched from the beginning of the path or not, but this fix assumes it. I started using gulp this morning, so excuse my lack of knowledge.

kvpt commented 10 years ago

I can confirm that this pull request doesn't work path.separator doesn't exist it's path.sep : http://nodejs.org/api/path.html#path_path_sep

sesponda commented 10 years ago

I'm sorry about the wrong variable name (separator vs. sep). I've fixed the pull request branch using "sep". I've also incorporated some of Cryszon's feedback above using a global regexp.

jamesknelson commented 10 years ago

Have you run the tests under windows for this? Do the tests actually cover it?

sesponda commented 10 years ago

I believe the tests do not cover this because they were running OK before and after my change. I haven't updated them.

jlas commented 10 years ago

Please see my tested fix/PR for this in Issue #5

jamesknelson commented 10 years ago

Closing as I've merged the fix in #5