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

Two issues i want to discuss #115

Open
island205 opened this issue Oct 24, 2015 · 7 comments
Open

Two issues i want to discuss #115

island205 opened this issue Oct 24, 2015 · 7 comments

Comments

@island205
Copy link

First, Same as this test case https://github.com/smysnk/gulp-rev-all/blob/master/test/fixtures/config1/index.html#L22, is there any case a file referenced by itself?

I can't find any case. But look this issue i came across:

file apply.js

$.ajax({
    url: "/apply",
    success: function() {
        $(this).addClass("done");
    }
});

after revision:

file apply.43uf8sfu9r.js

$.ajax({
    url: "/apply.43uf8sfu9r",
    success: function() {
        $(this).addClass("done");
    }
});

The ajax uri has been modified which i wasn't expect. we can fix it by adding following code in function resolveReferences:

// For the current file (fileResolveReferencesIn), look for references to any other file in the project
for (var path in this.files) {

    // Organize them by relative vs absolute reference types
    var fileCurrentReference = this.files[path];
    var references;

    // Don't resolve reference itself
    if (fileCurrentReference.revPathOriginal === fileResolveReferencesIn.revPathOriginal) {
      continue;
    }

Second, if i have some code like:

#set($scripts =["$!staticBasePath/js/eqb/register-success.js"])

regExps generated by function referenceToRegexs doesn't match "$!staticBasePath/js/eqb/register-success.js", because the prefix $!staticBasePath doesn't match nonFileNameChar which is /[^a-zA-Z0-9\\.\\-\\_\\/]/.

my solution is custom referenceToRegexs:

  var revAll = new RevAll({
    referenceToRegexs: function(reference) {
      var nonFileNameChar = '[^a-zA-Z0-9\\.\\-\\_\\/]';
      var qoutes = '\'|"';
      var escapedRefPathBase = Tool.path_without_ext(reference.path).replace(/([^0-9a-z])/ig, '\\$1');
      var escapedRefPathExt = Path.extname(reference.path).replace(/([^0-9a-z])/ig, '\\$1');

      var regExp, regExps = [];

        var isJSReference = reference.path.match(/\.js$/);

        // Extensionless javascript file references has to to be qouted
        if (isJSReference) {
            regExp = '('+ qoutes +')(' + escapedRefPathBase + ')()('+ qoutes + '|$)';
            regExps.push(new RegExp(regExp, 'g'));
        }

      // Expect left and right sides of the reference to be a non-filename type character, escape special regex chars
      regExp = '('+ nonFileNameChar +')(' + escapedRefPathBase + ')(' +  escapedRefPathExt + ')('+ nonFileNameChar + '|$)';
      regExps.push(new RegExp(regExp, 'g'));

      //handle staticBasePath
      regExp = '('+ nonFileNameChar +'\\$\\!staticBasePath)(' + escapedRefPathBase + ')(' +  escapedRefPathExt + ')('+ nonFileNameChar + '|$)';
      regExps.push(new RegExp(regExp, 'g'));

      return regExps;
    }
  });

but i don't like it due to long config.

what do you think, could we have some change to improve gulp-rev-all ?

@circlingthesun
Copy link
Contributor

Self references will occur in the wild so this is not a valid solution. See #106.

As for the 2nd issue, why not just replace set($scripts =["$!staticBasePath/js/eqb/register-success.js"]) with set($scripts =["$!" + "staticBasePath/js/eqb/register-success.js"])?

@island205
Copy link
Author

Any real self reference case? if there are some the code may go wrong in those cases.

2nd issue, $!staticBasePath is a variable return from the controller in spring mvc.

@circlingthesun
Copy link
Contributor

Well, say you a link to home.html on every page of your site. Its not unlikely that home.html will contain a link to itself. In that case, things will break.

As for the 2nd case, I can't really speak for the project, but if you put together a PR that elegantly deals with such edge cases, @smysnk might consider it.

@island205
Copy link
Author

indeed it is, but no one wants to rename the html files. i surely use dontRenameFile: ['.html'].

For the 2nd issue, I would have a try.

Thank you!

@circlingthesun
Copy link
Contributor

I revision my html files ;)

@vinnymac
Copy link

I attest @circlingthesun, I would revision them as well. However adding a flag to optionally not resolve references of filenames may not be uncalled for. Another solution may be to add a blacklist for files you don't want to find references based on the filename for. While html is likely to need a revision, if you had an application.js you may not want that reference revved, as it is more unlikely that the javascript is referencing itself. (the same argument could be made for an application.css, which may imply a whitelist instead)

@mikegleasonjr
Copy link

I think the 2nd issue is similar to #88

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

4 participants