Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

javascript newlines in strings need to be preserved in include filters #934

Merged
merged 3 commits into from

2 participants

@eleith

when including javascript files (unlike markdown) replacing newlines will cause javascript syntax errors. given

strings.js

var x = "here is \n some \n newlines";

and test.jade

html
   body
      includes strings.js

it will produce this file that contains javascript syntax errors

<html>
   <body><script>var x = "here is
 some
 newslines";
</script>
   </body>
</html>

this pull request contains a fix (only for javascript files, i don't know if other filters need this or not) as well as a unit test to ensure this behavior is preserved.

@ForbesLindesay

The escaping's currently a bit of a mess. I think we need to fix this by not double escaping in filters and instead only doing it when we actually need to (which isn't here)

@eleith

where is the double escaping happening?

or are you suggesting moving all the escaping of newlines into the individual filters themselves?

i'm happy to try and fix it in an acceptable way or just limit the PR to only the test, to show a case where js filter breaks.

i'm very much interested in getting this fixed.

@ForbesLindesay

I would happily accept the tests as a separate pull request as it's clearly something which should be working.

As for the double escaping, most filters double escape everything. e.g. markdown on lib/filters.js#L89. This is because it has to be double escaped here but as a result we have to undo that escaping here where it doesn't need to be escaped.

What I'm proposing we should do is remove JavaScript escaping from all the filters (by which I mean newline escaping, \ escaping etc. but not ' to &#39; which is a different kind of escaping) then we could just do the escaping in the one place where we actually need it, which is line 419 of the compiler.

@eleith

i've made an attempt to simplify and unify the escaping as you prescribed.

i hope the tests are as good as they seem...

@ForbesLindesay

Looks good to me. Tests can never catch everything, but jade's tests are fairly extensive.

@ForbesLindesay ForbesLindesay merged commit efcceea into jadejs:master

1 check passed

Details default The Travis build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 20, 2013
  1. only replace new lines if not a js file

    eleith authored
  2. tests

    eleith authored
Commits on Mar 24, 2013
This page is out of date. Refresh to see the latest.
View
6 lib/filters.js
@@ -39,7 +39,7 @@ exports.stylus = function(str, options){
var stylus = require('stylus');
stylus(str, options).render(function(err, css){
if (err) throw err;
- ret = css.replace(/\n/g, '\\n');
+ ret = css;
});
return '<style type="text/css">' + ret + '</style>';
};
@@ -53,7 +53,7 @@ exports.less = function(str){
str = str.replace(/\\n/g, '\n');
require('less').render(str, function(err, css){
if (err) throw err;
- ret = '<style type="text/css">' + css.replace(/\n/g, '\\n') + '</style>';
+ ret = '<style type="text/css">' + css + '</style>';
});
return ret;
};
@@ -86,7 +86,7 @@ exports.markdown = function(str){
}
str = str.replace(/\\n/g, '\n');
- return md.parse(str).replace(/\n/g, '\\n').replace(/'/g,'&#39;');
+ return md.parse(str).replace(/'/g,'&#39;');
};
/**
View
3  lib/parser.js
@@ -467,7 +467,8 @@ Parser.prototype = {
var str = fs.readFileSync(path, 'utf8').replace(/\r/g, '');
var ext = extname(path).slice(1);
var filter = filters[ext];
- if (filter) str = filter(str, { filename: path }).replace(/\\n/g, '\n');
+ if (filter) str = filter(str, { filename: path });
+
return new nodes.Literal(str);
}
View
2  lib/utils.js
@@ -49,7 +49,7 @@ var escape = exports.escape = function(str) {
*/
exports.text = function(str){
- return interpolate(escape(str));
+ return interpolate(escape(str)).replace(/\n/g, '\\n');
};
/**
View
5 test/cases/includes-with-ext-js.html
@@ -0,0 +1,5 @@
+<html>
+ <body><script>var x = "\n here is some \n new lined text";
+</script>
+ </body>
+</html>
View
3  test/cases/includes-with-ext-js.jade
@@ -0,0 +1,3 @@
+html
+ body
+ include javascript-new-lines.js
View
1  test/cases/javascript-new-lines.js
@@ -0,0 +1 @@
+var x = "\n here is some \n new lined text";
Something went wrong with that request. Please try again.