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

Fix embedded templates handling in block-indentation rule #2838

Merged
merged 2 commits into from
Mar 15, 2023
Merged

Fix embedded templates handling in block-indentation rule #2838

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
50c1bad
Add `columnOffset` to template infos for use in embedded templates
robinborst95 Mar 8, 2023
23e6fa3
Fix `block-indentation` rule for embedded templates
robinborst95 Mar 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 11 additions & 3 deletions lib/extract-templates.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { parseTemplates } from 'ember-template-imports/lib/parse-templates.js';
import * as util from 'ember-template-imports/src/util.js';

export const SUPPORTED_EXTENSIONS = ['.js', '.ts', '.gjs', '.gts'];
const LOCATION_START = Object.freeze({ line: 0, column: 0, start: 0, end: 0 });
const LOCATION_START = Object.freeze({ line: 0, column: 0, start: 0, end: 0, columnOffset: 0 });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way I approached it, is that the location of the embedded template is offset by the indentation of the line the template starts at. So, something like

test('it renders', async (assert) => {
  await render(hbs`

has an offset of 2, because await has an indentation of 2 spaces. Based on this info, each rule (block-indentation in this case, but attribute-indentation can probably work the same) can adjust the expected indentation if needed.

/**
* Processes results and corrects for template location offsets.
*
Expand Down Expand Up @@ -86,12 +86,18 @@ export function extractTemplates(moduleSource, relativePath) {
*
* @returns {TemplateInfo}
*/
function makeTemplateInfo({ line, column, start, end }, template, templateInfo, isEmbedded) {
function makeTemplateInfo(
{ line, column, start, end, columnOffset },
template,
templateInfo,
isEmbedded
) {
return {
line,
start,
end,
column,
columnOffset,
template,
isEmbedded,
isStrictMode: !isEmbedded || isStrictMode(templateInfo),
Expand All @@ -118,11 +124,13 @@ export function isSupportedScriptFileExtension(filePath = '') {

export function coordinatesOf(text, offset, start, end) {
const contentBeforeTemplateStart = text.slice(0, offset).split('\n');
const lineBeforeTemplateStart = contentBeforeTemplateStart[contentBeforeTemplateStart.length - 1];
return {
line: contentBeforeTemplateStart.length,
column: contentBeforeTemplateStart[contentBeforeTemplateStart.length - 1].length,
column: lineBeforeTemplateStart.length,
start,
end,
columnOffset: lineBeforeTemplateStart.length - lineBeforeTemplateStart.trimStart().length,
};
}

Expand Down
2 changes: 2 additions & 0 deletions lib/linter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,7 @@ export default class Linter {
let rule = this._buildRule(ruleName, {
shouldFix: true,
filePath: options.filePath,
columnOffset: templateInfo.columnOffset,
rawSource: templateInfo.template,
fileConfig,
});
Expand Down Expand Up @@ -367,6 +368,7 @@ export default class Linter {
fileConfig,
filePath: options.filePath,
configResolver: options.configResolver,
columnOffset: templateInfo.columnOffset,
rawSource: templateInfo.template,
isStrictMode: templateInfo.isStrictMode,
});
Expand Down
2 changes: 2 additions & 0 deletions lib/rules/_base.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ export default class {
this[MODULE_NAME] = options.moduleName;
this._rawSource = options.rawSource;

this.columnOffset = options.columnOffset;

// split into a source array (allow windows and posix line endings)
this.source = options.rawSource.match(reLines);
}
Expand Down
51 changes: 38 additions & 13 deletions lib/rules/block-indentation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,12 @@ function get(node, path) {
return value;
}

function removeWhitespaceEnd(str) {
return str.replace(/ +$/, '');
function addWhitespaceEnd(str, count) {
return `${str}${' '.repeat(count)}`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See line 326. The direct child indentation used to only remove whitespaces, but in embedded templates adding whitespaces is now also necessary sometimes.

}

function removeWhitespaceEnd(str, count) {
return str.replace(new RegExp(` {${count}}$`), '');
}

export default class BlockIndentation extends Rule {
Expand Down Expand Up @@ -306,13 +310,20 @@ export default class BlockIndentation extends Rule {
const hasLeadingContent =
!isDirectChildOfTemplate || this.hasLeadingContent(node, this.template.body);
const startColumn = node.loc.start.column;
const expectedStartColumn =
this.columnOffset === 0 || node.loc.start.line === 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 'root' node starting at line 1 is meant for cases like:

test('it renders', async (assert) => {
  await render(hbs`<div class="parent">
    <div class="child"></div>
  </div>`);
);

? 0
: this.columnOffset + this.config.indentation;

// If it's not a direct child of the template the block start is already fixed by the validateBlockChildren function
if (isDirectChildOfTemplate && startColumn !== 0 && !hasLeadingContent) {
if (isDirectChildOfTemplate && startColumn !== expectedStartColumn && !hasLeadingContent) {
if (this.mode === 'fix') {
const previousNode = this.template.body[indexOfNodeInTemplate - 1];
if (previousNode.type === 'TextNode') {
previousNode.chars = removeWhitespaceEnd(previousNode.chars);
previousNode.chars =
startColumn > expectedStartColumn
? removeWhitespaceEnd(previousNode.chars, startColumn - expectedStartColumn)
: addWhitespaceEnd(previousNode.chars, expectedStartColumn - startColumn);
}
} else {
let isElementNode = node && node.type === 'ElementNode';
Expand All @@ -322,7 +333,7 @@ export default class BlockIndentation extends Rule {

let warning =
`Incorrect indentation for \`${display}\` beginning at ${startLocation}. ` +
`Expected \`${display}\` to be at an indentation of 0, but was found at ${startColumn}.`;
`Expected \`${display}\` to be at an indentation of ${expectedStartColumn}, but was found at ${startColumn}.`;

this.log({
message: warning,
Expand All @@ -347,8 +358,12 @@ export default class BlockIndentation extends Rule {

let controlCharCount = this.endingControlCharCount(node);
let correctedEndColumn = endColumn - displayName.length - controlCharCount;
let expectedEndColumn =
this.columnOffset === 0 || node.loc.end.line !== this.template.loc.end.line
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar to the start of a block:

test('it renders', async (assert) => {
  await render(hbs`<div class="parent">
    <div class="child"></div>
  </div>`);
);

When the node it at the end of the template is and the template is embedded, then the start column should be the columnOffset

? startColumn
: this.columnOffset;

if (correctedEndColumn !== startColumn) {
if (correctedEndColumn !== expectedEndColumn) {
if (this.mode === 'fix') {
// If we are in a child (path set) we want to edit directly the source and not the source of the node
const elementSource = path
Expand All @@ -362,7 +377,7 @@ export default class BlockIndentation extends Rule {
lines,
node.loc.end.line - (path ? 1 : node.loc.start.line),
correctedEndColumn,
startColumn,
expectedEndColumn,
path
);
return fixedNode;
Expand All @@ -372,7 +387,7 @@ export default class BlockIndentation extends Rule {

let warning =
`Incorrect indentation for \`${displayName}\` beginning at ${startLocation}` +
`. Expected \`${display}\` ending at ${endLocation} to be at an indentation of ${startColumn} but ` +
`. Expected \`${display}\` ending at ${endLocation} to be at an indentation of ${expectedEndColumn} but ` +
`was found at ${correctedEndColumn}.`;

this.log({
Expand Down Expand Up @@ -405,7 +420,9 @@ export default class BlockIndentation extends Rule {
}

let startColumn = node.loc.start.column;
let expectedStartColumn = startColumn + this.config.indentation;
let correctedStartColumn =
this.columnOffset > 0 && node.loc.start.line === 1 ? this.columnOffset : startColumn;
let expectedStartColumn = correctedStartColumn + this.config.indentation;
let fixedNode = node;

let i = 0;
Expand Down Expand Up @@ -477,7 +494,7 @@ export default class BlockIndentation extends Rule {

fixedNode = this.fixLine(
lines,
childStartLine - 1,
childStartLine - (path ? 1 : fixedNode.loc.start.line),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This actually seemed to be an existing bug. When I ran the autofixer with master on our own embedded templates, then I got TypeError: Cannot read property 'startsWith' of undefined because the childStartLine was wrong

childStartColumn,
expectedStartColumn,
path
Expand Down Expand Up @@ -520,9 +537,11 @@ export default class BlockIndentation extends Rule {

let inverse = node.inverse;
let startColumn = node.loc.start.column;
let expectedStartColumn =
this.columnOffset > 0 && node.loc.start.line === 1 ? this.columnOffset : startColumn;
let elseStartColumn = node.program.loc.end.column;

if (elseStartColumn !== startColumn) {
if (elseStartColumn !== expectedStartColumn) {
if (this.mode === 'fix') {
// If we are in a child (path set) we want to edit directly the source and not the source of the node
const sourceToFix = path
Expand All @@ -533,7 +552,13 @@ export default class BlockIndentation extends Rule {
});
const lines = sourceToFix.split('\n');
const elseLine = node.program.loc.end.line;
const fixedNode = this.fixLine(lines, elseLine - 1, elseStartColumn, startColumn, path);
const fixedNode = this.fixLine(
lines,
elseLine - 1,
elseStartColumn,
expectedStartColumn,
path
);
return fixedNode;
} else {
let displayName = node.path.original;
Expand All @@ -542,7 +567,7 @@ export default class BlockIndentation extends Rule {

let warning =
`Incorrect indentation for inverse block of \`{{#${displayName}}}\` beginning at ${startLocation}` +
`. Expected \`{{else}}\` starting at ${elseLocation} to be at an indentation of ${startColumn} but ` +
`. Expected \`{{else}}\` starting at ${elseLocation} to be at an indentation of ${expectedStartColumn} but ` +
`was found at ${elseStartColumn}.`;

this.log({
Expand Down
8 changes: 8 additions & 0 deletions test/unit/extract-templates-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ describe('extractTemplates', function () {
[
{
"column": 0,
"columnOffset": 0,
"end": 0,
"isEmbedded": undefined,
"isStrictMode": true,
Expand All @@ -41,6 +42,7 @@ describe('extractTemplates', function () {
[
{
"column": 60,
"columnOffset": 6,
"end": 238,
"isEmbedded": true,
"isStrictMode": false,
Expand Down Expand Up @@ -71,6 +73,7 @@ describe('extractTemplates', function () {
[
{
"column": 60,
"columnOffset": 6,
"end": 242,
"isEmbedded": true,
"isStrictMode": true,
Expand Down Expand Up @@ -109,6 +112,7 @@ describe('extractTemplates', function () {
[
{
"column": 63,
"columnOffset": 6,
"end": 255,
"isEmbedded": true,
"isStrictMode": true,
Expand Down Expand Up @@ -139,6 +143,7 @@ describe('extractTemplates', function () {
[
{
"column": 39,
"columnOffset": 0,
"end": 58,
"isEmbedded": true,
"isStrictMode": true,
Expand Down Expand Up @@ -174,6 +179,7 @@ describe('extractTemplates', function () {
[
{
"column": 0,
"columnOffset": 0,
"end": 0,
"isEmbedded": undefined,
"isStrictMode": true,
Expand All @@ -190,6 +196,7 @@ describe('extractTemplates', function () {
[
{
"column": 39,
"columnOffset": 0,
"end": 58,
"isEmbedded": true,
"isStrictMode": true,
Expand Down Expand Up @@ -237,6 +244,7 @@ describe('calculate template coordinates', function () {
expect(coordinatesOf(typescript, 228)).toEqual({
line: 8,
column: 12,
columnOffset: 2,
});
});
});
Expand Down