Skip to content

Commit

Permalink
refactor glimmer post-process, better handle template tag (#1795)
Browse files Browse the repository at this point in the history 
* refactor glimmer post-process, better handle template tag

* try to re-trigger tests

* fix lint errors

* add support for multi-line eslint errors

* remove debugger

* address feedback

* Update lib/preprocessors/glimmer.js

Co-authored-by: Lucy Lin <lucy.ly.lin@gmail.com>

---------

Co-authored-by: Henry Majoros <hmajoros@linkedin.com>
Co-authored-by: Lucy Lin <lucy.ly.lin@gmail.com>
  • Loading branch information
@hmajoros @lin-ll
3 people committed Mar 9, 2023
1 parent e31728c commit 65fe1a4
Show file tree
Hide file tree
Showing 2 changed files with 184 additions and 33 deletions.
139 changes: 108 additions & 31 deletions lib/preprocessors/glimmer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,19 @@ function escapeRegExp(string) {
return string.replace(/[$()*+.?[\\\]^{|}]/g, '\\$&'); // $& means the whole matched string
}

function arrayEq(arr1, arr2) {
return arr1.length === arr2.length && arr1.every((val, idx) => val === arr2[idx]);
}

// regex to capture entire template with placeholder tags
const oneLineTemplateRegex = /\[__GLIMMER_TEMPLATE\(`(.*)`, {.*}\)]/;

// regex to capture opening template tag
const openingTemplateTagRegex = /\[__GLIMMER_TEMPLATE\(`$/;

// regex to capture closing template tag
const closingTemplateTagRegex = /`, {.*}\)]/;

/**
* This function is responsible for running the embedded templates transform
* from ember-template-imports.
Expand Down Expand Up @@ -101,49 +114,113 @@ function mapRange(messages, filename) {
return flattened;
}

// if there are no instances of the placeholder value, then this template
// did not contain a <template> tag, so preprocessed === postprocessed
if (!transformed.includes(util.TEMPLATE_TAG_PLACEHOLDER)) {
return flattened;
}

const lines = transformed.split('\n');
const originalLines = original.split('\n');

// this function assumes the original output and transformed output
// are identical, minus the changes to transform `<template>` into the
// placeholder `[__GLIMMER_TEMPLATE()]
//
// in the future, for a usecase like integrating prettier --fix,
// this assumption will no longer be valid, and should be removed/refactored
if (lines.length !== originalLines.length) {
throw new Error(
'eslint-plugin-ember expected source file and transform file to have the same line length, but they differed'
);
}

return flattened.map((message) => {
if (lines[message.line - 1] === originalLines[message.line - 1]) {
// original line and transformed line match exactly. return original diagnostic message
return message;
}
// 1. handle eslint diagnostics on single lines.
if (message.line === message.endLine) {
const originalLine = originalLines[message.line - 1];
const transformedLine = lines[message.line - 1];
if (originalLine === transformedLine) {
// original line and transformed line match exactly. return original diagnostic message
return message;
}

const line = lines[message.line - 1];
const token = line.slice(message.column - 1, message.endColumn - 1);

// Now that we have the token, we need to find the location
// in the original text
//
// TODO: Long term, we should use glimmer syntax parsing to find
// the correct node -- otherwise it's too easy to
// partially match on similar text
let originalLineNumber = 0;
let originalColumnNumber = 0;

for (const [index, line] of originalLines.entries()) {
const column = line.search(new RegExp(`\\b${escapeRegExp(token)}\\b`));
if (column > -1) {
originalLineNumber = index + 1;
originalColumnNumber = column + 1;
break;
// when the lines do not match, we've hit a lint error on the line containing the
// <template> tag, meaning we need to re-work the message
const modified = { ...message };
let token = transformedLine.slice(message.column - 1, message.endColumn - 1);

// lint error is specifically on JUST the opening <template> tag
if (openingTemplateTagRegex.test(token)) {
token = `<${util.TEMPLATE_TAG_NAME}>`;

// this case is simple: we know the starting column will be correct, so we just
// need to adjust the endColumn for the difference in length between the transformed
// token and the original token.
modified.endColumn = modified.column + token.length;
} else if (oneLineTemplateRegex.test(token)) {
// lint error is on a full, one-line <template>foo</template>
const templateContext = token.match(oneLineTemplateRegex)[1];
token = `<${util.TEMPLATE_TAG_NAME}>${templateContext}<${util.TEMPLATE_TAG_NAME}>`;

// this case is simple: we know we have a one-line template invocation, and the
// start `column` will be the same regardless of syntax. simply calculate the
// length of the full token `<template>...</template>` and set the endColumn.
modified.endColumn = modified.column + token.length;
} else {
// if we haven't fallen into one of the cases above, we are likely dealing with
// a scope token in the template placeholder, typically for being used but not
// defined. the `token` should be the specific template API which the error is on,
// so we need to find the usage of the token in the template

// TODO: this is still bug-prone, and should be refactored to do something like:
// 1. for given token, find associated template tag
// 2. conduct search for token only within associated template
// 3. ensure we are not matching token inside comments
for (const [index, line] of originalLines.entries()) {
const newColumn = line.search(new RegExp(`\\b${escapeRegExp(token)}\\b`));
if (newColumn > -1) {
modified.line = index + 1;
modified.column = newColumn + 1;
modified.endColumn = newColumn + token.length + 1;
break;
}
}
}
}

const modified = {
...message,
line: originalLineNumber,
column: originalColumnNumber,
endLine: originalLineNumber,
endColumn: originalColumnNumber + token.length,
};
return modified;
} else {
// 2. handle multi-line diagnostics from eslint
const originalRange = originalLines.slice(message.line - 1, message.endLine);
const transformedRange = lines.slice(message.line - 1, message.endLine);
if (arrayEq(originalRange, transformedRange)) {
// original line range and transformed linerange match exactly. return original diagnostic message
return message;
}

return modified;
// for ranges containing template tag placeholders, the only change we should need to make is
// on the endColumn field, as this is the only one guaranteed to be different. The start column
// field does not need to be transformed, because it should be the same regardless of if we are
// using a <template> tag or the placeholder.
//
// we modify the endColumn below by finding the index of the closing placeholder tag, and
// mapping the placeholder syntax back into the ending column of the original `</template>` tag

const modified = { ...message };

const endLine = lines[message.endLine - 1];
if (closingTemplateTagRegex.test(endLine)) {
const originalEndLine = originalLines[message.endLine - 1];
const closingTemplateTag = `</${util.TEMPLATE_TAG_NAME}>`;
const closingTagIndex = originalEndLine.indexOf(closingTemplateTag);

if (closingTagIndex) {
modified.endColumn = closingTagIndex + closingTemplateTag.length;
}
}

return modified;
}
});
}

Expand Down
78 changes: 76 additions & 2 deletions tests/lib/rules-preprocessor/gjs-gts-processor-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ function initESLint(options) {
rules: {
'lines-between-class-members': 'error',
'no-undef': 'error',
'no-unused-vars': 'error',
'ember/no-get': 'off',
'ember/no-array-prototype-extensions': 'error',
},
Expand Down Expand Up @@ -145,6 +146,47 @@ const invalid = [
},
],
},
{
filename: 'my-component.gjs',
code: `
import Component from '@glimmer/component';
export default class MyComponent extends Component {
foo = "bar";
<template>"hi"</template>
}
`,
errors: [
{
message: 'Expected blank line between class members.',
line: 6,
endLine: 6,
column: 9,
endColumn: 33,
},
],
},
{
filename: 'my-component.gjs',
code: `
import Component from '@glimmer/component';
export default class MyComponent extends Component {
foo = "bar";
<template>"hi"
</template>
}
`,
errors: [
{
message: 'Expected blank line between class members.',
line: 6,
endLine: 7,
column: 9,
endColumn: 19,
},
],
},
];

describe('template-vars', () => {
Expand Down Expand Up @@ -207,7 +249,7 @@ describe('line/col numbers should be correct', () => {
const code = `
import Component from '@glimmer/component';
import { get } from '@ember/object';
export default class MyComponent extends Component {
constructor() {
super(...arguments);
Expand Down Expand Up @@ -270,7 +312,38 @@ describe('lint errors on the exact line as the <template> tag', () => {
foo = 'bar';
<template>
<div>
some totally random, non-meaningful text
some totally random, non-meaningful text
</div>
</template>
}
`;
const results = await eslint.lintText(code, { filePath: 'my-component.gjs' });

const resultErrors = results.flatMap((result) => result.messages);
expect(resultErrors).toHaveLength(1);
expect(resultErrors[0].message).toBe('Expected blank line between class members.');
});
});

describe('multiple tokens in same file', () => {
it('correctly maps duplicate <template> tokens to the correct lines', async () => {
const eslint = initESLint();
const code = `
import Component from '@glimmer/component';
export const fakeTemplate = <template>
<div>"foo!"</div>
</template>
export default class MyComponent extends Component {
constructor() {
super(...arguments);
}
foo = 'bar';
<template>
<div>
some totally random, non-meaningful text
</div>
</template>
}
Expand All @@ -280,5 +353,6 @@ describe('lint errors on the exact line as the <template> tag', () => {
const resultErrors = results.flatMap((result) => result.messages);
expect(resultErrors).toHaveLength(1);
expect(resultErrors[0].message).toBe('Expected blank line between class members.');
expect(resultErrors[0].line).toBe(14);
});
});

0 comments on commit 65fe1a4

Please sign in to comment.