From 9cd4517a64cdfad64c297a0722c4304b19a52ff1 Mon Sep 17 00:00:00 2001 From: Christopher Chedeau Date: Mon, 17 Apr 2017 08:35:12 -0700 Subject: [PATCH] Fix template literal comments (#1296) The implementation was checking if the comment was inside of the expression range, which seems like a good idea. Unfortunately, the expression range is not what's inside of `${}` but the actual AST node, which incidentally doesn't include comments. So the logic was off and returned `undefined` which threw afterwards. Another solution is to find the first quasi where start is > comment start. This means that the comment appeared between the quasi before and this one... therefore in the expression before! The flow parser has issues with unicode where it makes node location invalid, there are likely other places where node locations are off. So instead of throwing with a weird error, let's attach it to the first one if it doesn't work. Fixes #1293 --- src/comments.js | 24 ++++++--------- .../template/__snapshots__/jsfmt.spec.js.snap | 30 +++++++++++++++++++ tests/template/comment.js | 12 ++++++++ 3 files changed, 51 insertions(+), 15 deletions(-) create mode 100644 tests/template/comment.js diff --git a/src/comments.js b/src/comments.js index 53010eec..243b1597 100644 --- a/src/comments.js +++ b/src/comments.js @@ -505,7 +505,7 @@ function handleObjectPropertyAssignment(enclosingNode, precedingNode, comment) { function handleTemplateLiteralComments(enclosingNode, comment) { if (enclosingNode && enclosingNode.type === "TemplateLiteral") { const expressionIndex = findExpressionIndexForComment( - enclosingNode.expressions, + enclosingNode.quasis, comment ); // Enforce all comments to be leading block comments. @@ -780,27 +780,21 @@ function printComment(commentPath) { } } -function findExpressionIndexForComment(expressions, comment) { - let match; +function findExpressionIndexForComment(quasis, comment) { const startPos = locStart(comment) - 1; - const endPos = locEnd(comment) + 1; - for (let i = 0; i < expressions.length; ++i) { - const range = getExpressionRange(expressions[i]); - - if ( - (startPos >= range.start && startPos <= range.end) || - (endPos >= range.start && endPos <= range.end) - ) { - match = i; - break; + for (let i = 1; i < quasis.length; ++i) { + if (startPos < getQuasiRange(quasis[i]).start) { + return i - 1; } } - return match; + // We haven't found it, it probably means that some of the locations are off. + // Let's just return the first one. + return 0; } -function getExpressionRange(expr) { +function getQuasiRange(expr) { if (expr.start !== undefined) { // Babylon return { start: expr.start, end: expr.end }; diff --git a/tests/template/__snapshots__/jsfmt.spec.js.snap b/tests/template/__snapshots__/jsfmt.spec.js.snap index d4f89e1f..f01ddd9d 100644 --- a/tests/template/__snapshots__/jsfmt.spec.js.snap +++ b/tests/template/__snapshots__/jsfmt.spec.js.snap @@ -1,5 +1,35 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP +exports[`comment.js 1`] = ` +\` +(?:\${escapeChar}[\\\\S\\\\s]|(?:(?!\${// Using \`XRegExp.union\` safely rewrites backreferences in \`left\` and \`right\`. +// Intentionally not passing \`basicFlags\` to \`XRegExp.union\` since any syntax +// transformation resulting from those flags was already applied to \`left\` and +// \`right\` when they were passed through the XRegExp constructor above. +XRegExp.union([left, right], '', {conjunction: 'or'}).source})[^\${escapeChar}])+)+ +\`; + +\`a\${/* b */c/* d */}e\${// f +g +// h +}\`; +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +\` +(?:\${escapeChar}[\\\\S\\\\s]|(?:(?!\${/* Using \`XRegExp.union\` safely rewrites backreferences in \`left\` and \`right\`.*/ +// Intentionally not passing \`basicFlags\` to \`XRegExp.union\` since any syntax +// transformation resulting from those flags was already applied to \`left\` and +// \`right\` when they were passed through the XRegExp constructor above. +XRegExp.union([left, right], "", { + conjunction: "or" +}).source})[^\${escapeChar}])+)+ +\`; + +\`a\${/* b */ /* d */ c}e\${/* f*/ +g}// h +\`; + +`; + exports[`faulty-locations.js 1`] = ` var o = { [\`key\`]: () => { diff --git a/tests/template/comment.js b/tests/template/comment.js new file mode 100644 index 00000000..8d09b338 --- /dev/null +++ b/tests/template/comment.js @@ -0,0 +1,12 @@ +` +(?:${escapeChar}[\\S\\s]|(?:(?!${// Using `XRegExp.union` safely rewrites backreferences in `left` and `right`. +// Intentionally not passing `basicFlags` to `XRegExp.union` since any syntax +// transformation resulting from those flags was already applied to `left` and +// `right` when they were passed through the XRegExp constructor above. +XRegExp.union([left, right], '', {conjunction: 'or'}).source})[^${escapeChar}])+)+ +`; + +`a${/* b */c/* d */}e${// f +g +// h +}`;