Skip to content

fix: unpaired apostrophe in JavaScript comments #370

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

Open
rtritto wants to merge 6 commits into
base: main
Choose a base branch
from
Open

fix: unpaired apostrophe in JavaScript comments #370

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ccd2f84
fix: unpaired apostrophe in JavaScript comments
rtritto Jun 4, 2026
b335b27
fix unpaired apostrophes within comments, and specific line terminato…
rtritto Jun 4, 2026
038fd66
hoisting the line-terminator regex outside the loop/function scope
rtritto Jun 4, 2026
c29c0ac
simplify the CRLF handling of the regex
rtritto Jun 4, 2026
bc1e1d1
add tests
rtritto Jun 5, 2026
34d8d6b
fix: handle unpaired apostrophes and improve closing tag parsing in c…
rtritto Jul 14, 2026
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
12 changes: 11 additions & 1 deletion src/parse.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ const singleQuoteReg = /'(?:\\[\s\w"'\\`]|[^\n\r'\\])*?'/g;

const doubleQuoteReg = /"(?:\\[\s\w"'\\`]|[^\n\r"\\])*?"/g;

const lineTerminatorReg = /(?:\r\n|[\n\r\u2028\u2029])/g;

@jayaddison jayaddison Jun 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This makes me think that we might be missing the U+2028 / U+2029 line terminators from the end-quote regex patterns (singleQuoteReg, doubleQuoteReg). I haven't explored this yet but would like to.

@jayaddison jayaddison Jun 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The regex itself seems correct to me. There might be a way to use the /.*/ pattern as an optimisation later; that should match on everything except for line terminators -- which are exactly the set of things this regex searches for.

The regex consumes CRLF when it is found, which is potentially helpful, because it'll move the search needle entirely past the linebreak to the following line's content.. although makes the code slightly less platform-agnostic.


/** Escape special regular expression characters inside a string */

function escapeRegExp(string: string) {
Expand Down Expand Up @@ -109,7 +111,7 @@ export function parse(this: Eta, str: string): Array<AstObject> {
);

const parseCloseReg = new RegExp(
"'|\"|`|\\/\\*|(\\s*(-|_)?" + escapeRegExp(config.tags[1]) + ")",
"'|\"|`|\\/\\*|\\/\\/|(\\s*(-|_)?" + escapeRegExp(config.tags[1]) + ")",
"g",
);

Expand Down Expand Up @@ -161,6 +163,14 @@ export function parse(this: Eta, str: string): Array<AstObject> {
ParseErr("unclosed comment", str, closeTag.index);
}
parseCloseReg.lastIndex = commentCloseInd;
} else if (char === "//") {
lineTerminatorReg.lastIndex = parseCloseReg.lastIndex;
const match = lineTerminatorReg.exec(str);
if (match) {
parseCloseReg.lastIndex = match.index + match[0].length;
} else {
parseCloseReg.lastIndex = str.length;
}
Comment thread
rtritto marked this conversation as resolved.
} else if (char === "'") {
singleQuoteReg.lastIndex = closeTag.index;

Expand Down
16 changes: 16 additions & 0 deletions test/parse.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,22 @@ describe("parse test", () => {
]);
});

it("works with unpaired apostrophe in single-line comment", () => {
const buff = eta.parse("hi <% // comment with unpaired apostrophe' \n %>");

@jayaddison jayaddison Jun 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

One more request for a test case: please could we add a (negative) test case where a single-line comment extends all the way to the end of the template?

In other words, something like:

const buff = eta.parse("hi <% // comment with unpaired apostrophe'");

This should raise an unclosed tag parse error exception.

The reason I'm asking: I think it might be possible to break out of one of the loops as a potential optimisation, at a later date. But let's add test coverage on the scenario to confirm the behaviour, before doing that.

expect(buff).toEqual([
"hi ",
{ val: "// comment with unpaired apostrophe' \n ", t: "e" },

@jayaddison jayaddison Jun 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@bgub I'm learning the codebase and parsing behaviour in order to provide feedback to @rtritto about this pull request.

From what I've understood so far: a parse result with t: "e" indicates a token that has type exec / evaluate. I think that means that the comment itself will be evaluated by the JavaScript engine.

That's probably fine, although I wonder: could we omit the parsed comments entirely, and skip evaluation of them? (saving a few evaluations, potentially for large/descriptive text such as license templates or lengthy explanatory comments?)

@jayaddison jayaddison Jun 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I now think that this would only affect comments inside template blocks - e.g. within <%...%> segments.

I wouldn't expect many or extensive comments within those, generally -- so perhaps there is not much likely performance improvement (in typical usage) available here.

]);
});

it("works with unpaired apostrophe in multiline comment", () => {
const buff = eta.parse("hi <% /* comment with unpaired apostrophe' */ %>");

@jayaddison jayaddison Jun 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@rtritto your fix works for double-quotes in addition to apostrophes, and I think that might be worth demonstrating too.

Another set of scenarios I'll look into is cases where there are mixed // and /* comments, including interleaving of those (for example -- start /* // middle */ end).

expect(buff).toEqual([
"hi ",
{ val: "/* comment with unpaired apostrophe' */ ", t: "e" },

@jayaddison jayaddison Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

NB: the parsed val result seems to be trimmed of surrounding whitespace:

Suggested change
{ val: "/* comment with unpaired apostrophe' */ ", t: "e" },
{ val: "/* comment with unpaired apostrophe' */", t: "e" },

(this affects the previous test case expectations too)

]);
});

it("parses with simple template literal", () => {
// biome-ignore lint/suspicious/noTemplateCurlyInString: intentional
const buff = eta.parse("hi <%= `template %> ${value}` %>");
Expand Down