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.

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

[gjs-gts-parser] fix type aware linting when using ts+gts files #1996

Merged
merged 1 commit into from
Dec 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
538 changes: 21 additions & 517 deletions lib/parsers/gjs-gts-parser.js
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit behind on this PR, so apologies while I ask a bunch of stupid questions 😅

I was looking at eslint-plugin-svelte, and they have something like this:

 {
      files: ['*.svelte'],
      parser: 'svelte-eslint-parser',
      parserOptions: {
        parser: {
          // Specify a parser for each lang.
          ts: '@typescript-eslint/parser',
          js: 'espree',
          typescript: '@typescript-eslint/parser'
        }
      }
    }

would that work for us / simplify this code?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we set a program, we also have to handle program updates... ts-eslint has a bunch of code fir that.
which is ehy i opted to pass a custom sys to ts.setSys.

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense, thanks!!!

Copy link
Contributor

Choose a reason for hiding this comment

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

well wait -- what do you mean by program updates?

Copy link

@vstefanovic97 vstefanovic97 Dec 7, 2023

Choose a reason for hiding this comment

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

@NullVoxPopuli either way it couldn't hurt to covert the code to ESM.
If you are up for it I can try to spend some time next couple of days and convert the codebase to ESM if you guys would be willing to review.
After that we can try Glint approach more easily and see if it will work? Wdyt? cc: @patricklx, @bmish

Copy link
Contributor

Choose a reason for hiding this comment

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

personally, I think since we're preparing for a major, now is a great time to convert to ESM -- (esp as that conversion is much easier to land than type-aware linting, which we still have lots to document and talk about (to spread knowledge, context, document issues, document with the TS team, etc))

Choose a reason for hiding this comment

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

@NullVoxPopuli I think we actually can't convert to ESM, because eslint package uses cjs so it won't be able to import parseForEslint function as well as rules if we converted them to ESM...
So I think unless glint packages start publishing cjs only approach we have is trying the dynamic import.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's correct. I tried to make an esm plugin for eslint once and it only works with the new configuration files.

Copy link
Member

Choose a reason for hiding this comment

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

Converting to ESM may be a large effort best saved for the next major version (not the in-progress v12 release which is already packed with breaking changes) and after ESLint v9 is out.

Large diffs are not rendered by default.

519 changes: 519 additions & 0 deletions lib/parsers/transform.js
Copy link
Contributor

Choose a reason for hiding this comment

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

Starting another unrelated thread, with some historical artefacts:

So, given this, I am not surprised making TS work with gjs/gts is a fair bit of work. And I have a hunch this is why the Vue ecosystem has optional support for jsx/tsx, because TS built in jsx/tsx into TS parsing with no option for other syntax plugins

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like none of this matters tho, because svelte seems to have fixed it all,
their config:

/** @type { import("eslint").Linter.FlatConfig } */
module.exports = {
	root: true,
	extends: [
		'eslint:recommended',
		'plugin:@typescript-eslint/recommended',
		'plugin:svelte/recommended',
		'prettier'
	],
	parser: '@typescript-eslint/parser',
	plugins: ['@typescript-eslint'],
	parserOptions: {
		sourceType: 'module',
		ecmaVersion: 2020,
		extraFileExtensions: ['.svelte']
	},
	env: {
		browser: true,
		es2017: true,
		node: true
	},
	overrides: [
		{
			files: ['*.svelte'],
			parser: 'svelte-eslint-parser',
			parserOptions: {
				parser: '@typescript-eslint/parser'
			}
		}
	]
};

Copy link
Contributor

@NullVoxPopuli NullVoxPopuli Dec 6, 2023

Choose a reason for hiding this comment

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

though, this requires generating a `.svelte-kit` folder before it can work for some reason

old comment, because they break general expectation of eslint and "just working on source files":

oh nevermind, they seem to actually be further behind: sveltejs/kit#11209

Large diffs are not rendered by default.

117 changes: 117 additions & 0 deletions lib/parsers/ts-utils.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
const fs = require('node:fs');
const { transformForLint } = require('./transform');
const babel = require('@babel/core');
const { replaceRange } = require('./transform');

let patchTs, replaceExtensions, syncMtsGtsSourceFiles, typescriptParser;

try {
const ts = require('typescript');
typescriptParser = require('@typescript-eslint/parser');
patchTs = function patchTs() {
const sys = { ...ts.sys };
const newSys = {
...ts.sys,
readDirectory(...args) {
const results = sys.readDirectory.call(this, ...args);
return [
...results,
...results.filter((x) => x.endsWith('.gts')).map((f) => f.replace(/\.gts$/, '.mts')),
];
},
fileExists(fileName) {
return fs.existsSync(fileName.replace(/\.mts$/, '.gts')) || fs.existsSync(fileName);
},
readFile(fname) {
let fileName = fname;
let content = '';
try {
content = fs.readFileSync(fileName).toString();
} catch {
fileName = fileName.replace(/\.mts$/, '.gts');
content = fs.readFileSync(fileName).toString();
}
if (fileName.endsWith('.gts')) {
content = transformForLint(content).output;
}
if (
(!fileName.endsWith('.d.ts') && fileName.endsWith('.ts')) ||
fileName.endsWith('.gts')
) {
content = replaceExtensions(content);
}
return content;
},
};
ts.setSys(newSys);
};

replaceExtensions = function replaceExtensions(code) {
let jsCode = code;
const babelParseResult = babel.parse(jsCode, {
parserOpts: { ranges: true, plugins: ['typescript'] },
});
const length = jsCode.length;
for (const b of babelParseResult.program.body) {
if (b.type === 'ImportDeclaration' && b.source.value.endsWith('.gts')) {
const value = b.source.value.replace(/\.gts$/, '.mts');
const strWrapper = jsCode[b.source.start];
jsCode = replaceRange(
jsCode,
b.source.start,
b.source.end,
strWrapper + value + strWrapper
);
}
}
if (length !== jsCode.length) {
throw new Error('bad replacement');
}
return jsCode;
};

/**
*
* @param program {ts.Program}
*/
syncMtsGtsSourceFiles = function syncMtsGtsSourceFiles(program) {
const sourceFiles = program.getSourceFiles();
for (const sourceFile of sourceFiles) {
// check for deleted gts files, need to remove mts as well
if (sourceFile.path.endsWith('.mts') && sourceFile.isVirtualGts) {
const gtsFile = program.getSourceFile(sourceFile.path.replace(/\.mts$/, '.gts'));
if (!gtsFile) {
sourceFile.version = null;
}
}
if (sourceFile.path.endsWith('.gts')) {
/**
* @type {ts.SourceFile}
*/
const mtsSourceFile = program.getSourceFile(sourceFile.path.replace(/\.gts$/, '.mts'));
if (mtsSourceFile) {
const keep = {
fileName: mtsSourceFile.fileName,
path: mtsSourceFile.path,
originalFileName: mtsSourceFile.originalFileName,
resolvedPath: mtsSourceFile.resolvedPath,
};
Object.assign(mtsSourceFile, sourceFile, keep);
mtsSourceFile.isVirtualGts = true;
}
}
}
};
} catch /* istanbul ignore next */ {
// typescript not available
patchTs = () => null;
replaceExtensions = (code) => code;
syncMtsGtsSourceFiles = () => null;
}

module.exports = {
patchTs,
replaceExtensions,
syncMtsGtsSourceFiles,
typescriptParser,
};
9 changes: 8 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@
]
},
"dependencies": {
"@babel/core": "^7.23.3",
"@babel/eslint-parser": "^7.22.15",
"@ember-data/rfc395-data": "^0.0.4",
"@glimmer/syntax": "^0.85.12",
Expand Down Expand Up @@ -116,7 +117,13 @@
"typescript": "^5.2.2"
},
"peerDependencies": {
"eslint": ">= 8"
"eslint": ">= 8",
"typescript": "*"
},
"peerDependenciesMeta": {
"typescript": {
"optional": true
}
},
"engines": {
"node": "18.* || 20.* || >= 21"
Expand Down
5 changes: 5 additions & 0 deletions tests/lib/rules-preprocessor/ember_ts/bar.gts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export const fortyTwoFromGTS = '42';

<template>
{{fortyTwoFromGTS}}
</template>
1 change: 1 addition & 0 deletions tests/lib/rules-preprocessor/ember_ts/baz.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const fortyTwoFromTS = '42';
14 changes: 14 additions & 0 deletions tests/lib/rules-preprocessor/ember_ts/foo.gts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { fortyTwoFromGTS } from './bar.gts';
import { fortyTwoFromTS } from './baz.ts';

export const fortyTwoLocal = '42';

const helloWorldFromTS = fortyTwoFromTS[0] === '4' ? 'hello' : 'world';
const helloWorldFromGTS = fortyTwoFromGTS[0] === '4' ? 'hello' : 'world';
const helloWorld = fortyTwoLocal[0] === '4' ? 'hello' : 'world';
//
<template>
{{helloWorldFromGTS}}
{{helloWorldFromTS}}
{{helloWorld}}
</template>
125 changes: 123 additions & 2 deletions tests/lib/rules-preprocessor/gjs-gts-parser-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@

const { ESLint } = require('eslint');
const plugin = require('../../../lib');
const { writeFileSync, readFileSync } = require('node:fs');
const { join } = require('node:path');

const gjsGtsParser = require.resolve('../../../lib/parsers/gjs-gts-parser');

Expand Down Expand Up @@ -388,18 +390,28 @@ const invalid = [
code: `
import Component from '@glimmer/component';

const foo: any = '';

export default class MyComponent extends Component {
foo = 'bar';

<template>
<div></div>${' '}
{{foo}}
</template>
}`,
errors: [
{
message: 'Unexpected any. Specify a different type.',
line: 4,
endLine: 4,
column: 18,
endColumn: 21,
},
{
message: 'Trailing spaces not allowed.',
line: 8,
endLine: 8,
line: 10,
endLine: 10,
column: 22,
endColumn: 24,
},
Expand Down Expand Up @@ -765,4 +777,113 @@ describe('multiple tokens in same file', () => {
expect(resultErrors[2].message).toBe("'bar' is not defined.");
expect(resultErrors[2].line).toBe(17);
});

it('lints while being type aware', async () => {
const eslint = new ESLint({
ignore: false,
useEslintrc: false,
plugins: { ember: plugin },
overrideConfig: {
root: true,
env: {
browser: true,
},
plugins: ['ember'],
extends: ['plugin:ember/recommended'],
overrides: [
{
files: ['**/*.gts'],
parser: 'eslint-plugin-ember/gjs-gts-parser',
parserOptions: {
project: './tsconfig.eslint.json',
tsconfigRootDir: __dirname,
extraFileExtensions: ['.gts'],
patricklx marked this conversation as resolved.
Show resolved Hide resolved
},
extends: [
'plugin:@typescript-eslint/recommended-requiring-type-checking',
'plugin:ember/recommended',
],
rules: {
'no-trailing-spaces': 'error',
'@typescript-eslint/prefer-string-starts-ends-with': 'error',
},
},
{
files: ['**/*.ts'],
parser: '@typescript-eslint/parser',

Choose a reason for hiding this comment

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

@patricklx don't we need to leave *.ts files parsing to eslint-plugin-ember/gjs-gts-parser also?

Choose a reason for hiding this comment

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

If we don't use it then I assume we will again have errors if we try to import from a .gts file inside a regular .ts file

Choose a reason for hiding this comment

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

Though this means that we will need to recommend ppl use eslint-plugin-ember/gjs-gts-parser for all files js/ts/gjs/gts in order to have it work properly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i decided now to just overwrite the TS.sys functions, typescript-eslint already handles those cases and with this we keep it like that

Choose a reason for hiding this comment

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

aha I see, so basically we just monkey patch ts directly. Might be a bit brittle but it will work :D.
And we don't need to worry about creating our own TS program nor involving glint :D
Solid idea!

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 problem i'm facing now is how to replace .gts without breaking some eslint rule...
especially the stylistic ones.
it would shift all code by 1 character... but adding something after it breaks some whitespace rules.
So now i use the extension .mts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Though this means that we will need to recommend ppl use eslint-plugin-ember/gjs-gts-parser for all files js/ts/gjs/gts in order to have it work properly

Maybe, now ts.sys is overwritten as soon as gjs-gts-parser is loaded. Which is loaded during config loading.

Choose a reason for hiding this comment

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

@patricklx yes you are right, with ts patch approach we can leave ts files to typescript-eslint/parser.
Again solid idea patching this :)

parserOptions: {
project: './tsconfig.eslint.json',
tsconfigRootDir: __dirname,
extraFileExtensions: ['.gts'],
},
extends: [
'plugin:@typescript-eslint/recommended-requiring-type-checking',
Copy link
Contributor

Choose a reason for hiding this comment

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

what's this config? I didn't see it listed here: https://typescript-eslint.io/linting/configs

Copy link
Member

Choose a reason for hiding this comment

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

It might be an old config name but it's for type-aware linting: https://typescript-eslint.io/linting/typed-linting/

Copy link
Contributor

Choose a reason for hiding this comment

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

'plugin:@typescript-eslint/recommended-type-checked',

vs

'plugin:@typescript-eslint/recommended-requiring-type-checking',

?

Copy link
Contributor

Choose a reason for hiding this comment

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

'plugin:ember/recommended',
],
rules: {
'no-trailing-spaces': 'error',
},
},
],
rules: {
quotes: ['error', 'single'],
semi: ['error', 'always'],
'object-curly-spacing': ['error', 'always'],
'lines-between-class-members': 'error',
'no-undef': 'error',
'no-unused-vars': 'error',
'ember/no-get': 'off',
'ember/no-array-prototype-extensions': 'error',
'ember/no-unused-services': 'error',
},
},
});

let results = await eslint.lintFiles(['**/*.gts', '**/*.ts']);

let resultErrors = results.flatMap((result) => result.messages);
expect(resultErrors).toHaveLength(3);

expect(resultErrors[0].message).toBe("Use 'String#startsWith' method instead.");
expect(resultErrors[0].line).toBe(6);

expect(resultErrors[1].line).toBe(7);
expect(resultErrors[1].message).toBe("Use 'String#startsWith' method instead.");

expect(resultErrors[2].line).toBe(8);
expect(resultErrors[2].message).toBe("Use 'String#startsWith' method instead.");

const filePath = join(__dirname, 'ember_ts', 'bar.gts');
const content = readFileSync(filePath).toString();
try {
writeFileSync(filePath, content.replace("'42'", '42'));

results = await eslint.lintFiles(['**/*.gts', '**/*.ts']);

resultErrors = results.flatMap((result) => result.messages);
expect(resultErrors).toHaveLength(2);

expect(resultErrors[0].message).toBe("Use 'String#startsWith' method instead.");
expect(resultErrors[0].line).toBe(6);

expect(resultErrors[1].line).toBe(8);
expect(resultErrors[1].message).toBe("Use 'String#startsWith' method instead.");
} finally {
writeFileSync(filePath, content);
}

results = await eslint.lintFiles(['**/*.gts', '**/*.ts']);

resultErrors = results.flatMap((result) => result.messages);
expect(resultErrors).toHaveLength(3);

expect(resultErrors[0].message).toBe("Use 'String#startsWith' method instead.");
expect(resultErrors[0].line).toBe(6);

expect(resultErrors[1].message).toBe("Use 'String#startsWith' method instead.");
expect(resultErrors[1].line).toBe(7);

expect(resultErrors[2].line).toBe(8);
expect(resultErrors[2].message).toBe("Use 'String#startsWith' method instead.");
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

can we also add a test for a failure case for a type-aware lint?

Like, any of

'@typescript-eslint/no-unsafe-argument': 'error',
'@typescript-eslint/no-unsafe-assignment': 'error',
'@typescript-eslint/no-unsafe-call': 'error',
'@typescript-eslint/no-unsafe-member-access': 'error',
'@typescript-eslint/no-unsafe-return': 'error',

5 changes: 3 additions & 2 deletions tests/lib/rules-preprocessor/tsconfig.eslint.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
"strictNullChecks": true
},
"include": [
"*"
]
"**/*.ts",
"**/*.gts"
],
}
Loading