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.

Sign up for GitHub

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

Jump to bottom

Triple backticks to allow creation of JavaScript blocks #4357

Merged
merged 9 commits into from
Nov 19, 2016
Merged

Triple backticks to allow creation of JavaScript blocks #4357

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
27e8a62
Support JavaScript code blocks set apart by triple backticks (``` ...…
GeoffreyBooth Nov 14, 2016
107d8af
Add test for escaped backticks
GeoffreyBooth Nov 14, 2016
edd36b5
Remove TODOs for things we’re never going to support
GeoffreyBooth Nov 14, 2016
b9dd310
Convert escaped backticks to backticks; update tests
GeoffreyBooth Nov 15, 2016
3200259
Block inline JavaScript can end with an escaped backtick character
GeoffreyBooth Nov 15, 2016
0d5449f
Updated JavaScript token regexes per @lydell
GeoffreyBooth Nov 15, 2016
7689620
In JavaScript blocks, escape backslashes when they immediately preced…
GeoffreyBooth Nov 17, 2016
9945e68
Test that we don’t break backslash escaping in JavaScript literals
GeoffreyBooth Nov 17, 2016
621d9d5
Merge branch 'master' into triple-backticks
GeoffreyBooth Nov 18, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions lib/coffee-script/lexer.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 10 additions & 3 deletions src/lexer.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,8 +293,15 @@ exports.Lexer = class Lexer
# Matches JavaScript interpolated directly into the source via backticks.
jsToken: ->
return 0 unless @chunk.charAt(0) is '`' and match = JSTOKEN.exec @chunk
@token 'JS', (script = match[0])[1...-1], 0, script.length
script.length
[js, here] = match
if here?
script = here
length = here.length + 6 # 6 is the length of the six ` characters
else
script = js[1...-1]
length = js.length
@token 'JS', script, 0, length
length

# Matches regular expression literals, as well as multiline extended ones.
# Lexing regular expressions is difficult to distinguish from division, so we
Expand Down Expand Up @@ -900,7 +907,7 @@ CODE = /^[-=]>/

MULTI_DENT = /^(?:\n[^\n\S]*)+/

JSTOKEN = /^`[^\\`]*(?:\\.[^\\`]*)*`/
JSTOKEN = /^```([\s\S]*?)```|^`[^\\`]*(?:\\.[^\\`]*)*`/
Copy link
Contributor

@greghuc greghuc Nov 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read the added regex part as "capture everything between starting and end triple backquotes, stopping at first-seen ending triple quotes", which seems sane.

The original regex is rather obtuse. I read it as "greedily read everything between starting and ending single backquotes, and make sure every backslash has a character following it". I'm thinking the backslash+character part exists to stop an escaped backslash preemptively closing the Javascript block. E.g:

OK: `a\``
Not OK: `a\`

The new triple backticks regex will behave differently:

OK: ```a\````
Also OK: ```a````

I found this website useful: https://regex101.com/

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#3760 is actually complaining about just what you describe: a backslash not followed by a character (in that case, it's followed by a newline). Perhaps the original regex is incorrect here?

What would you use as the regex, if you don't mind taking the time to play with it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parsing this way is inconsistent with heredocs and heregexen, where backslash can be used to escape characters adjacent to the delimeters:

"""\"hello\"""" # '"hello"'
///\/hello\//// # /\/hello\//
```\`hello\```` # ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per my last comment, I’m suggesting we convert \`` to ``` wherever `` appears inside a single- or triple-backtick block. That should cover this case, yes?


# String-matching-regexes.
STRING_START = /^(?:'''|"""|'|")/
Expand Down
36 changes: 29 additions & 7 deletions test/javascript_literals.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,32 @@
# Javascript Literals
# JavaScript Literals
# -------------------

# TODO: refactor javascript literal tests
# TODO: add indexing and method invocation tests: `[1]`[0] is 1, `function(){}`.call()
test "inline JavaScript is evaluated", ->
eq '\\`', `
// Inline JS
"\\\`"
`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With my proposed escaping rules, this test needs to be written like this:

eq '\\`', `
  // Inline JS
  "\\\\\`"
`

or like this:

eq '\\a`', `
  // Inline JS
  "\\a\`"
`


eq '\\`', `
// Inline JS
"\\\`"
`
test "escaped backticks are output correctly", ->
`var a = 'foo\`bar';`
eq a, 'foo`bar'

test "block inline JavaScript is evaluated", ->
```
var a = 1;
var b = 2;
```
c = 3
```var d = 4;```
eq a + b + c + d, 10

test "block inline JavaScript containing backticks", ->
```
// This is a comment with `backticks`
var a = 42;
var b = `foo ${'bar'}`;
Copy link
Contributor

@greghuc greghuc Nov 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no 'eq' for variable b in the ""block inline JavaScript containing backticks" test. Is this correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, as I didn't want to rely on ES2015 in the 1.x branch. Though I guess I am already for it to parse the b line. Hmm...

var c = 3;
var d = 'foo`bar`';
```
eq a + c, 45
eq d, 'foo`bar`'