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

Merged
merged 3 commits into from Mar 24, 2013

Conversation

Projects
None yet
2 participants

eleith commented Mar 20, 2013

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.

Owner

ForbesLindesay commented Mar 23, 2013

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 commented Mar 23, 2013

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.

Owner

ForbesLindesay commented Mar 24, 2013

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 commented Mar 24, 2013

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

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

Owner

ForbesLindesay commented Mar 24, 2013

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

ForbesLindesay added a commit that referenced this pull request Mar 24, 2013

Merge pull request #934 from eleith/patch-includes-javascript-newlines
javascript newlines in strings need to be preserved in include filters

@ForbesLindesay ForbesLindesay merged commit efcceea into pugjs:master Mar 24, 2013

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment