Use custom parser for gts/gjs #1920

patricklx · 2023-07-27T11:51:01Z

I was writing this parser for eslint-plugin-template-lint,
but I think it makes sense to have it for the ember-plugin as well.
Also, as preprocess changes the filename to a virtual one, type aware linting will not work: typescript-eslint/typescript-eslint#6177 (comment)
By using a parser, that would be fixed
bonus:

prettier eslint plugin (with template tag prettier plugin) will just work for gts/gjs
semi rule works same as prettier tag plugin
enables type aware lints
can detect unused block params in templates
can detect undef vars in PathExpression
can add eslint directive comments in mustache or html

disadvantage:

prettier will not work without template tag prettier plugin for gts/gjs files

fixes #1770
fixes #1894
fixes #1747
fixes #1683
fixes #1659
also fixes ember-tooling/prettier-plugin-ember-template-tag#81
fixes ember-tooling/prettier-plugin-ember-template-tag#84
closes #1667

patricklx · 2023-07-27T12:06:50Z

.github/workflows/ci.yml

@@ -17,7 +17,7 @@ jobs:
    strategy:
      matrix:
        os: [ ubuntu, windows ]
-        node-version: [14.x, 16.x, 18.x]


because of @typescript-eslint/scope-manager

This'll require a major release to drop node 14, cc @bmish

I can also try to downgrade to 5.62.0.
They dropped 14 with the 6.0.0 release

i downgraded as i also needed lower version for testing with typed eslint

NullVoxPopuli · 2023-08-03T12:20:46Z

package.json

    "css-tree": "^2.0.4",
    "ember-rfc176-data": "^0.3.15",
-    "ember-template-imports": "^3.4.2",
-    "ember-template-recast": "^6.1.4",
+    "ember-template-tag": "^2.2.3",


any reason to not use content-tag?

content-tag only transforms the content. it does not provide any info about the changes done.
It also does not guarantee the same code syntax & spacing, as its not a recast and also adds import statement at the top, which changes all lines.
i tried also to use source mapping, but it doesn't matter if the syntax, semi & spacing is changed. which causes tokens to be different of the created AST. which is used by eslint to detect e.g. semi, spacing issues etc

patricklx · 2023-08-04T18:50:28Z

I don't think we can use content-tag unless it transforms a file into a useable form for linting.

patricklx · 2023-08-04T18:56:26Z

Anyone else who can review? @bmish ?

NullVoxPopuli · 2023-08-08T12:12:10Z

.github/workflows/ci.yml

@@ -17,7 +17,7 @@ jobs:
    strategy:
      matrix:
        os: [ ubuntu, windows ]
-        node-version: [14.x, 16.x, 18.x]


This'll require a major release to drop node 14, cc @bmish

NullVoxPopuli · 2023-08-08T12:15:17Z

lib/parsers/gjs-parser.js

+    const { templateVisitorKeys } = preprocessedResult;
+    const visitorKeys = { ...result.visitorKeys, ...templateVisitorKeys };
+    result.isTypescript = isTypescript;
+    convertAst(result, preprocessedResult, visitorKeys);


does this mean that the template-lint rules could be added as eslint rules? I saw you had @glimmer/syntax as well?

yes, I'm already having most of them working with this parser over at eslint-plugin-template-lint. running them natively. though only on my pc :)
and fixing is another issue. since template-lint works on ast. but block-indentation fixes things in a mixed way... and does trailing whitespace fixes, new line fixes, not just indentation

NullVoxPopuli · 2023-08-08T12:15:58Z

lib/parsers/gjs-parser.js

+ * @param preprocessedResult
+ * @param visitorKeys
+ */
+function convertAst(result, preprocessedResult, visitorKeys) {


why is all this code needed?
do we need to zip in the glimmer AST with the JS AST?
does doing so allow us to eventually phase out template-lint? (or adopt the rules in to here?)

yes, possibly. but mostly now to auto-detect unused and undef vars. Also helps in existing rules as you saw:)
And later for eslint plugin template lint

NullVoxPopuli · 2023-08-08T12:17:05Z

lib/rules/no-unused-services.js

@@ -380,6 +363,13 @@ module.exports = {
          currentClass.uses.add(name);
        }
      },
+      GlimmerPathExpression(node) {


this is much nicer!

NullVoxPopuli · 2023-08-08T12:17:25Z

package.json

    "css-tree": "^2.0.4",
    "ember-rfc176-data": "^0.3.15",
-    "ember-template-imports": "^3.4.2",


good to see this gone

NullVoxPopuli · 2023-08-08T12:18:04Z

tests/lib/rules-preprocessor/gjs-gts-processor-test.js

@@ -472,6 +499,40 @@ describe('lint errors on the exact line as the <template> tag', () => {
  });
 });

+describe('supports eslint directives inside templtes', () => {


Suggested change

describe('supports eslint directives inside templtes', () => {

describe('supports eslint directives inside templates', () => {

NullVoxPopuli · 2023-08-08T12:20:03Z

lib/index.js

 module.exports = {
  rules: requireIndex(`${__dirname}/rules`),
  configs: requireIndex(`${__dirname}/config`),
  utils: {
    ember: require('./utils/ember'),
  },
-  processors: {


we still probably want to export the parser -- @chancancode had a use case where the (now old) parser breaks on old code -- it'd be good for us to provide a way for folks to configure their own overrides in the lint config and set the parser:

overrides: [ { files: ['modern/**/*.gts'], parser: require('eslint-plugin-ember/gts-parser'), // ... } ]

(we'd need to use package.json#exports to expose this tho -- maybe for another PR)

Or rather have an option to specify the internal base parser.
Now I'm just choosing between Babel parser and typescript eslint parser. But that could also be an option passed to the gts parser

NullVoxPopuli · 2023-08-09T12:03:52Z

tests/lib/rules-preprocessor/my-component.gts

Empty file?

NullVoxPopuli · 2023-08-09T12:05:00Z

tests/lib/rules-preprocessor/gjs-gts-processor-test.js

@@ -27,13 +30,28 @@ function initESLint(parser = '@babel/eslint-parser') {
      env: {
        browser: true,
      },
-      parser,
      parserOptions: {


Oh i see how those files are used legit

patricklx · 2023-08-10T21:30:04Z

trailing spaces also work inside template tag, but they also remove them inside pre tag, textarea tag.
We could provide a rule which extends/wraps the base rule and just skips the check inside those tags.
For indent a took a look how typescript lint does it. And they wrap the eslint rule and call its function.
So i saw that eslint already supports jsxelement, which we can call to get indent working. It works because it in the end only operates on tokens. same can also be done for quotes
I already have it working here:
https://github.com/patricklx/eslint-plugin-ember-template-lint/blob/convert-lint-rules/lib/rules/indentation.js#L220
I will also make work for attributes.
Should those 2 rules be part of this plugin? I would do that in another pr

bonus: * enables type aware lints * prettier eslint plugin (with template tag prettier plugin) will just work for gts/gjs * can detect unused block params in templates * can detect undef vars in PathExpression * can add eslint directive comments in mustache or html disadvantage: * prettier will not work without template tag prettier plugin for gts/gjs files

bmish

Thanks a lot for this!

Techn1x · 2023-08-22T10:24:40Z

Bumped to this and I seem to get an eslint error at the very start of all of my <template> tags now 🤔

This is defined within a class

export class Alert extends Component<AlertSignature> {
  <template> ... </template>
}

I also get an error in all my TOC components

Parsing error: '>' expected

Any ideas?

Relevant packages installed;
eslint 8.47
eslint-plugin-ember 11.11.0
ember-template-lint 5.11.2
ember-template-lint-plugin-prettier 5.0.0
prettier-plugin-ember-template-tag 1.0.2

patricklx · 2023-08-22T10:30:28Z

@Techn1x do you have specified a parser in your eslint config? Like the typescript parser?
If so, then remove it and extend from 'plugin:@typescript-eslint/base'. That will set it up correctly and not interfere with emberjs parser

bmish · 2023-08-22T11:19:25Z

@Techn1x can you open a separate issue to discuss this? @patricklx if this is a breaking change, we will need to revert it.

Techn1x · 2023-08-22T11:20:03Z

Thanks, will do.

This reverts commit 9bd5ee4.

bmish · 2023-08-22T12:21:40Z

This has been reverted in:

Revert "Use custom parser for gts/gjs (#1920)" #1941

patricklx force-pushed the use-custom-parser branch 7 times, most recently from c9bb26d to 876e76d Compare July 27, 2023 12:06

patricklx commented Jul 27, 2023

View reviewed changes

patricklx force-pushed the use-custom-parser branch 5 times, most recently from 9ee801f to 0ce038c Compare August 1, 2023 09:51

patricklx closed this Aug 1, 2023

patricklx reopened this Aug 1, 2023

patricklx force-pushed the use-custom-parser branch 3 times, most recently from 808c7bf to 212d4d5 Compare August 3, 2023 11:43

NullVoxPopuli reviewed Aug 3, 2023

View reviewed changes

patricklx force-pushed the use-custom-parser branch 2 times, most recently from c168c29 to 0128f9c Compare August 4, 2023 10:26

patricklx mentioned this pull request Aug 5, 2023

New rule to lint against referencing let variables in <template> #1897

Closed

patricklx force-pushed the use-custom-parser branch from 0128f9c to f494221 Compare August 7, 2023 07:52

bmish requested review from NullVoxPopuli and lin-ll August 7, 2023 09:35

patricklx force-pushed the use-custom-parser branch 2 times, most recently from 06421d1 to 1bad26b Compare August 8, 2023 09:38

NullVoxPopuli approved these changes Aug 8, 2023

View reviewed changes

NullVoxPopuli reviewed Aug 9, 2023

View reviewed changes

tests/lib/rules-preprocessor/my-component.gts Outdated

Copy link

Contributor

NullVoxPopuli Aug 9, 2023

Choose a reason for hiding this comment

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

Empty file?

NullVoxPopuli reviewed Aug 9, 2023

View reviewed changes

NullVoxPopuli approved these changes Aug 9, 2023

View reviewed changes

patricklx force-pushed the use-custom-parser branch 2 times, most recently from 13262a6 to 1932886 Compare August 10, 2023 07:32

patricklx force-pushed the use-custom-parser branch from 1932886 to 8682c87 Compare August 11, 2023 14:23

patricklx force-pushed the use-custom-parser branch from 8682c87 to d6aa734 Compare August 16, 2023 07:55

bmish added the enhancement label Aug 21, 2023

bmish changed the title ~~use custom parser for gts/gjs~~ Use custom parser for gts/gjs Aug 21, 2023

bmish approved these changes Aug 21, 2023

View reviewed changes

bmish merged commit 9bd5ee4 into ember-cli:master Aug 21, 2023
8 checks passed

patricklx deleted the use-custom-parser branch August 21, 2023 13:39

Techn1x mentioned this pull request Aug 22, 2023

Parsing error for template tags within classes on v11.11.0 #1940

Closed

bmish added a commit to bmish/eslint-plugin-ember that referenced this pull request Aug 22, 2023

Revert "Use custom parser for gts/gjs (ember-cli#1920)"

6b3ac3f

This reverts commit 9bd5ee4.

bmish mentioned this pull request Aug 22, 2023

Revert "Use custom parser for gts/gjs (#1920)" #1941

Merged

bmish added a commit that referenced this pull request Aug 22, 2023

Revert "Use custom parser for gts/gjs (#1920)" (#1941)

256a704

patricklx restored the use-custom-parser branch August 22, 2023 12:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use custom parser for gts/gjs #1920

Use custom parser for gts/gjs #1920

patricklx commented Jul 27, 2023 •

edited

Loading

patricklx Jul 27, 2023

NullVoxPopuli Aug 8, 2023

patricklx Aug 8, 2023

patricklx Aug 9, 2023

NullVoxPopuli Aug 3, 2023

patricklx Aug 3, 2023 •

edited

Loading

patricklx commented Aug 4, 2023

patricklx commented Aug 4, 2023

NullVoxPopuli Aug 8, 2023

NullVoxPopuli Aug 8, 2023

patricklx Aug 8, 2023 •

edited

Loading

NullVoxPopuli Aug 8, 2023

patricklx Aug 8, 2023 •

edited

Loading

NullVoxPopuli Aug 8, 2023

NullVoxPopuli Aug 8, 2023

NullVoxPopuli Aug 8, 2023

patricklx Aug 8, 2023

NullVoxPopuli Aug 8, 2023

NullVoxPopuli Aug 8, 2023

patricklx Aug 8, 2023

NullVoxPopuli Aug 9, 2023

NullVoxPopuli Aug 9, 2023

patricklx commented Aug 10, 2023 •

edited

Loading

bmish left a comment

Techn1x commented Aug 22, 2023 •

edited

Loading

patricklx commented Aug 22, 2023 •

edited

Loading

bmish commented Aug 22, 2023

Techn1x commented Aug 22, 2023

bmish commented Aug 22, 2023

	describe('supports eslint directives inside templtes', () => {
	describe('supports eslint directives inside templates', () => {

Use custom parser for gts/gjs #1920

Use custom parser for gts/gjs #1920

Conversation

patricklx commented Jul 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patricklx Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

patricklx commented Aug 4, 2023

patricklx commented Aug 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patricklx Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patricklx Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patricklx commented Aug 10, 2023 • edited Loading

bmish left a comment

Choose a reason for hiding this comment

Techn1x commented Aug 22, 2023 • edited Loading

patricklx commented Aug 22, 2023 • edited Loading

bmish commented Aug 22, 2023

Techn1x commented Aug 22, 2023

bmish commented Aug 22, 2023

patricklx commented Jul 27, 2023 •

edited

Loading

patricklx Aug 3, 2023 •

edited

Loading

patricklx Aug 8, 2023 •

edited

Loading

patricklx Aug 8, 2023 •

edited

Loading

patricklx commented Aug 10, 2023 •

edited

Loading

Techn1x commented Aug 22, 2023 •

edited

Loading

patricklx commented Aug 22, 2023 •

edited

Loading