fix: unpaired apostrophe in JavaScript comments by rtritto · Pull Request #370 · bgub/eta

rtritto · 2026-06-04T18:04:31Z

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR updates the Eta template parser to recognize and skip JavaScript-style single-line (// ...) comments while scanning template code blocks, preventing premature tag-closing detection inside line comments.

Changes:

Extend the close-token regex to detect // comment starts.
Add logic to advance the parser index to the next line terminator when // is encountered.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…rs in JavaScript comments

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

jayaddison · 2026-06-05T09:41:17Z

@rtritto I think we'll want test coverage in parse.spec.ts to demonstrate the problem and confirm the fix.

Also: I am worried about edge cases such as open-close syntax elements (such as the apostrophe, double-quote, and so on) that themselves contain comments -- e.g. a string such as 'this is the start of the string and there is a comment /* here */ within it' -- or variants of that.

I'll try to find some time to explore other examples that might cause the parser to misinterpret the input; trying to find those should allow us to make the parser more robust.

rtritto · 2026-06-05T10:11:13Z

@jayaddison add some test on parse.spec.ts

jayaddison · 2026-06-22T14:44:47Z

Sorry for not getting to this sooner; I am beginning to examine the code for similar problems now. Thank you for adding some test coverage.

jayaddison · 2026-06-22T15:27:36Z

+    const buff = eta.parse("hi <% // comment with unpaired apostrophe' \n %>");
+    expect(buff).toEqual([
+      "hi ",
+      { val: "// comment with unpaired apostrophe' \n ", t: "e" },


@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?)

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.

jayaddison · 2026-06-22T15:30:06Z

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


@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).

jayaddison · 2026-06-22T16:54:23Z

NB: I think that this changeset also resolves #336 (a closed bug, although replicable against v4.6.0 during my testing).

jayaddison · 2026-06-22T18:04:09Z


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

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


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 · 2026-06-22T18:21:49Z


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

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


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.

jayaddison · 2026-06-22T18:32:27Z

  });

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


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.

jayaddison · 2026-06-24T13:20:26Z

+    const buff = eta.parse("hi <% /* comment with unpaired apostrophe' */ %>");
+    expect(buff).toEqual([
+      "hi ",
+      { val: "/* comment with unpaired apostrophe' */ ", t: "e" },


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)

jayaddison · 2026-06-24T13:27:28Z

I've been thinking about why the string-and-comment parsing logic was introduced/updated, particularly after looking at commit 78c256c.

I think it's so that templates can contain strings/comments that themselves include the same symbols used for template tags (e.g. <% and %>).

So: some test coverage for situations like that could also make sense; it's not necessarily directly related to the fix here, so we should probably do that in a separate pull request.

cc @bgub, partly to check if my theory about the parsing logic is anywhere near accurate

jayaddison

I think that this fixes the bug, so from that point of view I'm ready to approve it.

However: my memory is that parsing of source code syntax is rarely safe using regular expressions, because most languages have context-sensitive syntax trees, where the behaviour (mode/context) of the parser changes as it reads and interprets certain tokens. Comments and string literals are two of those.

The eta parser does produce a context-type indicator (e.g. i, e, ...) along with each value, but without knowing that it is/uses a generalised JavaScript parser, I don't have strong confidence that all language syntax quirks are captured -- and that could mean that there are ways to confuse/bypass the regex patterns and inject or obscure the behaviour of code.

So I am prepared to approve this fix in-the-small (after the test cases are updated/expanded), but I would personally remain cautious about using it in important production code.

It is possible that there exist collections of JavaScript and other language code that are designed to be adversarial and break parsers/interpreters in unexpected ways -- I don't know of any of those, but ideally I would suggest testing a parser against a large-ish collection of cases like that (sometimes called adversarial datasets because they intentionally go against-the-grain of the language/implementers, to try to find bugs).

jayaddison · 2026-06-24T13:40:10Z

(I have not used any AI/LLMs for this review; I know that my terminology/language is not perfect, and I may have misunderstood some aspects of parsing, and also aspects of eta -- I've tried my best)

bgub

Thanks for addressing #337. The core approach fixes the reported unpaired-apostrophe case, but I don't think this is ready to merge yet.

Please address these before merging:

The two new tests currently fail. Eta trims whitespace immediately before the closing tag, but the assertions expect that whitespace to remain in val. Please correct the expectations and make sure the full suite passes.
The new // regex alternative runs before the configured close-tag alternative. This regresses custom closing delimiters beginning with // when they are adjacent to code. For example, this works on main but throws unclosed tag on this branch:

new Eta({ tags: ["{{", "//"] }).parse("{{= it.x//")

The same issue occurs with a delimiter such as //}}. Please preserve the existing custom-delimiter behavior and add a regression test.

Please also add passing coverage for the line-comment termination cases this implementation explicitly handles—at minimum \r, \r\n, U+2028, and U+2029—plus a close-tag-looking sequence inside a single-line comment.

For reference, I ran pnpm lint and pnpm build successfully. pnpm test failed only on the two newly added assertions (107 tests passed). Performance looks neutral for ordinary templates and slightly better for comment-heavy templates, so performance is not a blocker.

…omments

rtritto · 2026-07-14T19:10:42Z

@bgub done

fix: unpaired apostrophe in JavaScript comments

ccd2f84

Copilot AI review requested due to automatic review settings June 4, 2026 18:04

Copilot AI reviewed Jun 4, 2026

View reviewed changes

Comment thread src/parse.ts

fix unpaired apostrophes within comments, and specific line terminato…

b335b27

…rs in JavaScript comments

rtritto requested a review from Copilot June 4, 2026 18:11

Copilot AI reviewed Jun 4, 2026

View reviewed changes

Comment thread src/parse.ts Outdated

Comment thread src/parse.ts Outdated

Comment thread src/parse.ts Outdated

rtritto added 2 commits June 4, 2026 20:24

hoisting the line-terminator regex outside the loop/function scope

038fd66

simplify the CRLF handling of the regex

c29c0ac

rtritto requested a review from Copilot June 4, 2026 18:27

Copilot AI reviewed Jun 4, 2026

View reviewed changes

rtritto mentioned this pull request Jun 4, 2026

Suggestion: revert back to lodash templates, albeit dropping use of the 'with' statement GoogleChrome/workbox#3529

Open

add tests

bc1e1d1

jayaddison reviewed Jun 22, 2026

View reviewed changes

jayaddison reviewed Jun 24, 2026

View reviewed changes

bgub requested changes Jul 14, 2026

View reviewed changes

fix: handle unpaired apostrophes and improve closing tag parsing in c…

34d8d6b

…omments


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

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

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

Uh oh!

Conversation

rtritto commented Jun 4, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

jayaddison commented Jun 5, 2026

Uh oh!

rtritto commented Jun 5, 2026

Uh oh!

jayaddison commented Jun 22, 2026

Uh oh!

jayaddison Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

jayaddison Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

jayaddison Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

jayaddison commented Jun 22, 2026

Uh oh!

jayaddison Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

jayaddison Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

jayaddison Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

jayaddison Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

jayaddison commented Jun 24, 2026

Uh oh!

jayaddison left a comment

Choose a reason for hiding this comment

Uh oh!

jayaddison commented Jun 24, 2026

Uh oh!

bgub left a comment

Choose a reason for hiding this comment

Uh oh!

rtritto commented Jul 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants