rotundasoftware / nunjucksify

Everything you expect from a module named nunjucksify and more.
34 stars 10 forks source link

Prevent duplicate require calls and simplify template compilation #4

Closed w0rm closed 10 years ago

w0rm commented 10 years ago

This bit of code doesn't check for duplicates

while( match = reg.exec( nunjucksCompiledStr ) ) {
    var templateRef = match[1];
    compiledTemplate += 'require( "' + templateRef + '" );\n';
}

Should be easy to fix, I'm just writing it down to not forget later. Don't have much time at the moment, but will open a pr if it is welcome :)

w0rm commented 10 years ago

@dgbeck hi!

I added simple check to prevent duplicate require calls for the same template and a test for it. I also added simple test that compares the result of nunjucksified template with nunjucks.render.

dgbeck commented 10 years ago

Hi @w0rm , the change here looks good but these tests are not passing for me. I'm getting

➜  nunjucksify git:(w0rm-4-prevent-duplicate-require-calls) mocha test

  ․․

  1 passing (618ms)
  1 failing

  1)  Renders the same in node and in dom:

      Uncaught AssertionError: "" == "<h1>Hello, world</h1>\n"
      + expected - actual

      +<h1>Hello, world</h1>

      at /Users/david_beck/Documents/git/nunjucksify/test/test.js:35:12
      at Stream.end (/Users/david_beck/Documents/git/nunjucksify/test/test.js:51:27)
      at _end (/Users/david_beck/Documents/git/nunjucksify/node_modules/through/index.js:65:9)
      at Stream.stream.end (/Users/david_beck/Documents/git/nunjucksify/node_modules/through/index.js:74:5)
      at Labeled.onend (/Users/david_beck/Documents/git/nunjucksify/node_modules/browserify/node_modules/labeled-stream-splicer/node_modules/stream-splicer/node_modules/readable-stream/lib/_stream_readable.js:530:10)
      at Labeled.g (events.js:180:16)
      at Labeled.EventEmitter.emit (events.js:117:20)
      at /Users/david_beck/Documents/git/nunjucksify/node_modules/browserify/node_modules/labeled-stream-splicer/node_modules/stream-splicer/node_modules/readable-stream/lib/_stream_readable.js:927:16
      at process._tickCallback (node.js:415:13)

Is there something I'm missing?

w0rm commented 10 years ago

@dgbeck This is strange because all tests are passing for me :(

w0rm commented 10 years ago

@dgbeck I merged the code that simplifies template compilation into this brunch because I added jsdom errors propagation there. Can you try to run tests once more? I hope that it will display a decent error message that explains why you got empty string.

dgbeck commented 10 years ago

Closing since #2 now includes these chanages