jsoverson / preprocess

Preprocess HTML, JavaScript, and other files with directives based off custom or ENV configuration
Other
366 stars 80 forks source link

In getTestTemplate() empty template is evaluated as true #46

Closed gabriellupu closed 9 years ago

gabriellupu commented 9 years ago

In preprocess.js the following method gets test = "%0D" (newline)

function getTestTemplate(test) {
  /*jshint evil:true*/
  test = test || 'true==true';

  // force single equals replacement
  test = test.replace(/([^=])=([^=])/g, '$1==$2');

  return new Function("context", "with (context||{}){ return ( " + test + " ) }");
}

this is causing test to be evaluated as true because it's a newline, and the

new Function("context", "with (context||{}){ return ( " + test + " ) }");

breaks with "SyntaxError: Unexpected token )". This happens only under Windows (8.1 x64), on MAC it runs fine.

Full stack

Running "preprocess:ironclad" (preprocess) task
Warning: Unexpected token ) Use --force to continue.
SyntaxError: Unexpected token )
    at Object.Function (<anonymous>)
    at getTestTemplate (D:\ironclad\node_modules\grunt-preprocess\node_modules\preprocess\lib\preprocess.js:123:10)
    at testPasses (D:\ironclad\node_modules\grunt-preprocess\node_modules\preprocess\lib\preprocess.js:127:16)
    at D:\ironclad\node_modules\grunt-preprocess\node_modules\preprocess\lib\preprocess.js:78:12
    at String.replace (native)
    at preprocess (D:\ironclad\node_modules\grunt-preprocess\node_modules\preprocess\lib\preprocess.js:77:11)
    at D:\ironclad\node_modules\grunt-preprocess\node_modules\preprocess\lib\preprocess.js:68:22
    at String.replace (native)
    at Object.preprocess (D:\ironclad\node_modules\grunt-preprocess\node_modules\preprocess\lib\preprocess.js:58:11)
    at D:\ironclad\node_modules\grunt-preprocess\tasks\preprocess.js:58:36

Also, the following test is failing:

Running "nodeunit:preprocess" (nodeunit) task
Testing preprocess_test.js..........F......
>> preprocess - include files
>> Message: Should include files and indent if ending with a newline
>> Error: 'a!foobar!\r\n c' == 'a!foobar!\n c'
>> at Object.exports.preprocess.include files (test\preprocess_test.js:443:10)
>> at Object.exports.preprocess.setUp (test\preprocess_test.js:35:5)

Warning: 1/76 assertions failed (26ms) Use --force to continue.

Aborted due to warnings.
gabriellupu commented 9 years ago

The actual problem was that UNIX prefers \n line ending style since Windows evaluates them as \r\n sometimes. The required update was to adapt a regex in preprocess.js from

/\n/g

to

/\r?\n/g

Check the pull request. Thanks !

PS: Tests are now passing on Linux and Windows as well now.

gabriellupu commented 9 years ago

I think this can be closed since PR #47 has been merged in.

jsoverson commented 9 years ago

thanks @lupugabriel1

brian428 commented 9 years ago

Sorry to have to re-open this, but I'm running into the same problem, but for an exclude test. I'm running a project that appears to end up passing a test string containing only a new line. I'm on Windows, so I think this is coming through as "\r\n". I'm not sure if something outside of preprocess.js is passing something it really shouldn't be, or if it's a result of the regex replacement that happens earlier in the script.

Either way, this blows up in getTestTemplate(). I was able to prevent it by adding a line at the top of testPasses() that contains: if( test && test.replace( /[\n\r\s]*/gi, '' ) == '' ) { test = '' }. (Obviously, this is just saying "after replacing all spaces, cr, and lf, if we're left with an empty string, then set test to an empty string").

I don't understand everything going on in preprocess.js well enough to know if this is a truly viable fix, but for now it's what I'm using to stop it from failing.

brian428 commented 9 years ago

Just checking to see if I should be opening a new issue for this?

ghost commented 9 years ago

I thought I had the same issue. It turned out I had a copy-paste issue with '// ifdef etc... -->' where I had left on an html tag on the end. It would be nice to have a little better logging info to find these types of syntax errors, as all it says is the aforementioned 'SyntaxError: Unexpected token )'. Hope this is helpful.