Use custom parser for gts/gjs #1942

patricklx · 2023-08-22T12:30:27Z

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

disavdvantage:
needs specific configuration to enable the parser, as setting it globally will overwrite it.
like https://github.com/vuejs/vue-eslint-parser does.

Breaking change:

semi rules for template part. by coincidence same as prettier.
no-undef rule will take effect for template vars (includes js scope)
no-unsed rule will take effect for template block params
new config for gjs/gts, notified if not setup correctly

e.g.

// .eslintrc.js
module.exports = {
  plugins: ['ember'],
  extends: [
    'eslint:recommended',
    'plugin:ember/recommended', // or other configuration
  ],
  overrides: [
    {
      files: ['**/*.gts', '**/*.gjs'],
      parser: 'eslint-plugin-ember/gjs-gts-parser'
    },
    }
  ],
  rules: {
    // override / enable optional rules
    'ember/no-replace-test-comments': 'error'
  }
};

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
closes #1957

NullVoxPopuli · 2023-08-22T14:04:52Z

README.md

+    // override / enable optional rules
+    'ember/no-replace-test-comments': 'error'
+  }
+};


I'd prefer we specify:

overrides: [ { files: ["**/*.{gjs,gts}"], parser: 'ember', } ]

would this also be okay?

{ files: ['**/*.gts'], parser: 'eslint-plugin-ember/gts-parser', }, { files: ['**/*.gjs'], parser: 'eslint-plugin-ember/gjs-parser', },

This seems fine, but just wondering, is there a need or benefit to separating the parsers, or can they be specified as one?

overrides: [ { files: ["**/*.{gjs,gts}"], parser: 'eslint-plugin-ember/gjs-gts-parser', } ]

we could.
we would need to detect if the extension is gts or gjs and choose by that, which internal parser should be chosen

Would that be simple? Is there any reason why we shouldn't just auto-detect the extension for the user? If the user doesn't care about distinguishing between these file types, then we can avoid exposing that complexity to the user, and just handle it internally, right?

patricklx · 2023-08-22T15:16:41Z

@bmish @NullVoxPopuli
I think there are two options

Use non breaking version and keep old code, but strange override
breaking version, and we keep the glimmer preprocessor which only throws an error notifying the user how to setup the parser.

Though i remember the idea was to get rid of ember-template-imports here.
So i will continue with breaking change and look how to notify users about the new configuration. I think i can get something working preprocessors.

patricklx · 2023-08-23T09:19:40Z

lib/preprocessors/noop.js

+      msgs[0].ruleId === null &&
+      msgs[0].message.startsWith('Parsing error: ')
+    ) {
+      if (!parsedFiles.has(fileName)) {


this is how we check if the file went through our parser or not.
I only notify the user if a parsing error happened. We could also always notify the user if its not correctly setup.

I think this is great! is there a way to check if a gjs / gts file was passed to this though? right now I have a concern about non-gjs and non-gts files hitting this message, would be over-aggressive

Well, the preprocessor is only setup for those files. So it only enters if for gjs/gts

patricklx · 2023-08-23T09:21:07Z

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

+    }`,
+    errors: [
+      {
+        message: 'Parsing error: Unexpected token (2:40)',


here the error when setup is correct, but some other syntax error

patricklx · 2023-08-23T09:25:24Z

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

+          'Parsing error: Unexpected token. A constructor, method, accessor, or property was expected.\n' +
+          'To lint Gjs/Gts files please follow the setup guide at https://github.com/ember-cli/eslint-plugin-ember#gtsgjs-eslint-setup',


here is the error when not setting it up correctly

patricklx · 2023-08-23T09:35:32Z

@bmish @NullVoxPopuli
I found a way to notify the users if their eslint config needs to be updated for gts/gjs and link them to the README.md.
So with this we should be able to make a breaking release and users will know what to do.
Or should I include the setup in the error message?

NullVoxPopuli · 2023-08-23T15:58:20Z

In your PR description, your eslint config uses the root namespace for configuring ember stuff -- I think we need to move the community more towards an overrides-only approach, which I'd been trying to get folks to care about for some time.

For example:

// .eslintrc.js
module.exports = {
  overrides: [
    {
      files: ['**/*.gts'],
      parser: 'eslint-plugin-ember/gts-parser',
      plugins: ['ember'],
      extends: [
        'eslint:recommended',
        'plugin:ember/recommended', // or other configuration
      ],
    },
    {
      files: ['**/*.gjs'],
      parser: 'eslint-plugin-ember/gjs-parser',
      plugins: ['ember'],
      extends: [
        'eslint:recommended',
        'plugin:ember/recommended', // or other configuration
      ],
    },
    {
      files: ['tests/**/*.{gjs,gts}'],
      rules: {
        // override / enable optional rules
        'ember/no-replace-test-comments': 'error'
      }
    }
  ],
};

Reason being that it's way easier to manage long term, as it flattens the amount of cascading you force yourself in to, and you can more easily opt out of plugins, because they never existed for a particular folder in the first place.

This aligns nicely with the direction ESLint9 is going in as the current config format will be dropped. 🙃

this is all tangential to the PR tho lol

NullVoxPopuli · 2023-08-23T16:01:37Z

lib/parsers/gjs-parser.js

+}
+
+/**
+ * simple tokenizer for templates, just splits it up into words and punctuators


can totally be in a followup PR, but does it makes sense for this to have its own unit tests?

Maybe, though once the indent rule is added, it will also be tested indirectly :)

NullVoxPopuli · 2023-08-23T16:03:43Z

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

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

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


NullVoxPopuli · 2023-08-23T16:04:09Z

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

+    const code = `
+    <template>
+      <div>
+        <!--eslint-disable-next-line-->


wouldn't this get shipped to users tho? 😅
(non-blocker, just a goofiness)

NullVoxPopuli

Nice work -- thanks for iterating on this, and greatest apologies we had to revert the first time around <3

patricklx · 2023-08-24T11:19:56Z

In your PR description, your eslint config uses the root namespace for configuring ember stuff -- I think we need to move the community more towards an overrides-only approach, which I'd been trying to get folks to care about for some time.

For example:
// .eslintrc.js
module.exports = {
  overrides: [
    {
      files: ['**/*.gts'],
      parser: 'eslint-plugin-ember/gts-parser',
      plugins: ['ember'],
      extends: [
        'eslint:recommended',
        'plugin:ember/recommended', // or other configuration
      ],
    },
    {
      files: ['**/*.gjs'],
      parser: 'eslint-plugin-ember/gjs-parser',
      plugins: ['ember'],
      extends: [
        'eslint:recommended',
        'plugin:ember/recommended', // or other configuration
      ],
    },
    {
      files: ['tests/**/*.{gjs,gts}'],
      rules: {
        // override / enable optional rules
        'ember/no-replace-test-comments': 'error'
      }
    }
  ],
};
Reason being that it's way easier to manage long term, as it flattens the amount of cascading you force yourself in to, and you can more easily opt out of plugins, because they never existed for a particular folder in the first place.

This aligns nicely with the direction ESLint9 is going in as the current config format will be dropped. 🙃

this is all tangential to the PR tho lol

That would also be good, by using extends in overrides it would not also not be strictly necessary to specify the parser. As that is set in the recommended config.
So only one config would be needed:

{
      files: ['**/*.{gjs,gts}'],
        plugins: ['ember'],
       extends: [
         'eslint:recommended',
         'plugin:ember/recommended', // and other 
}

Or a base config?
edit: the multiple configs actual make more sense, i might want to have different rules for gjs/gts as gts could also have the typescript ones.
should we have different recommended configs for gjs/gts? there we could add rules that work on template and not have them in the js/ts files...

patricklx · 2023-08-29T07:14:03Z

follow-up prs i would like to include in the next major:

patricklx · 2023-10-19T04:55:15Z

@bmish any news for this?
Ember template imports 4 has been released.

NullVoxPopuli · 2023-10-19T13:28:39Z

eti v4 is more a signal that content-tag should be good to go in the linting prs.

patricklx · 2023-10-19T13:45:45Z

content tag still doesn't emit a format that's usable for linting

NullVoxPopuli · 2023-10-19T13:54:20Z

It has a second api specifically for linting. Check my glint pr where the ranges are used

patricklx · 2023-10-19T14:02:59Z

I will have a look, but here ranges are not enough.
Need to have a transform which doesn't change offsets

patricklx · 2023-10-19T14:16:38Z

mmm, so glint uses that to do it's own transform?
it's also possible to know if the template is inside a class expression or normal expression?
If so, then i could also do the custom transform here...
@NullVoxPopuli ?

patricklx · 2023-10-23T13:27:30Z

@bmish @NullVoxPopuli I updated this to use the latest content-tag

NullVoxPopuli

Looks great!

NullVoxPopuli · 2023-10-31T12:58:32Z

@patricklx this one needs rebase real quick

lib/parsers/gjs-gts-parser.js

package.json

patricklx · 2023-11-01T11:09:52Z

@NullVoxPopuli this now uses latest glimmer/syntax

patricklx force-pushed the use-custom-parser branch 2 times, most recently from 2578221 to a96dc3b Compare August 22, 2023 12:52

NullVoxPopuli reviewed Aug 22, 2023

View reviewed changes

patricklx force-pushed the use-custom-parser branch 5 times, most recently from 2bb7972 to 71d19ff Compare August 22, 2023 14:49

patricklx force-pushed the use-custom-parser branch 6 times, most recently from 4eff4a0 to e8c8be6 Compare August 23, 2023 09:17

patricklx commented Aug 23, 2023

View reviewed changes

patricklx force-pushed the use-custom-parser branch from e8c8be6 to e50fde7 Compare August 23, 2023 09:22

patricklx commented Aug 23, 2023

View reviewed changes

patricklx force-pushed the use-custom-parser branch from e50fde7 to 2260f0c Compare August 23, 2023 13:20

This was referenced Aug 23, 2023

Add new rule template-indent #1943

Merged

Add new rule template-no-let-reference #1939

Merged

NullVoxPopuli reviewed Aug 23, 2023

View reviewed changes

NullVoxPopuli previously approved these changes Aug 23, 2023

View reviewed changes

gitKrystan mentioned this pull request Oct 19, 2023

Replace ember-template-imports with content-tag ember-tooling/prettier-plugin-ember-template-tag#149

Closed

1 task

patricklx force-pushed the use-custom-parser branch from 5981ad7 to 767ba8d Compare October 23, 2023 13:26

NullVoxPopuli approved these changes Oct 23, 2023

View reviewed changes

patricklx mentioned this pull request Oct 31, 2023

Plan v12 Release #1535

Closed

patricklx force-pushed the use-custom-parser branch 2 times, most recently from aca80ed to 8b53ff0 Compare October 31, 2023 13:05

patricklx commented Oct 31, 2023

View reviewed changes

lib/parsers/gjs-gts-parser.js Show resolved Hide resolved

NullVoxPopuli reviewed Oct 31, 2023

View reviewed changes

package.json Outdated Show resolved Hide resolved

patricklx force-pushed the use-custom-parser branch 8 times, most recently from 4d93fa9 to df57176 Compare November 1, 2023 10:54

use custom parser

9dbfa05

patricklx force-pushed the use-custom-parser branch from df57176 to 9dbfa05 Compare November 1, 2023 10:56

NullVoxPopuli approved these changes Nov 1, 2023

View reviewed changes

NullVoxPopuli merged commit 068609d into ember-cli:master Nov 1, 2023

bmish changed the title ~~use custom parser for gts/gjs~~ Use custom parser for gts/gjs Nov 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use custom parser for gts/gjs #1942

Use custom parser for gts/gjs #1942

patricklx commented Aug 22, 2023 •

edited

Loading

NullVoxPopuli Aug 22, 2023 •

edited

Loading

patricklx Aug 22, 2023

NullVoxPopuli Aug 22, 2023

bmish Aug 31, 2023

patricklx Aug 31, 2023

bmish Aug 31, 2023

patricklx commented Aug 22, 2023 •

edited

Loading

patricklx Aug 23, 2023

NullVoxPopuli Aug 23, 2023

patricklx Aug 23, 2023 •

edited

Loading

patricklx Aug 23, 2023

patricklx Aug 23, 2023

patricklx commented Aug 23, 2023 •

edited

Loading

NullVoxPopuli commented Aug 23, 2023

NullVoxPopuli Aug 23, 2023

patricklx Aug 23, 2023

NullVoxPopuli Aug 23, 2023

NullVoxPopuli Aug 23, 2023

NullVoxPopuli left a comment •

edited

Loading

patricklx commented Aug 24, 2023 •

edited

Loading

patricklx commented Aug 29, 2023

patricklx commented Oct 19, 2023

NullVoxPopuli commented Oct 19, 2023

patricklx commented Oct 19, 2023

NullVoxPopuli commented Oct 19, 2023

patricklx commented Oct 19, 2023

patricklx commented Oct 19, 2023 •

edited

Loading

patricklx commented Oct 23, 2023

NullVoxPopuli left a comment

NullVoxPopuli commented Oct 31, 2023

patricklx commented Nov 1, 2023

		'Parsing error: Unexpected token. A constructor, method, accessor, or property was expected.\n' +
		'To lint Gjs/Gts files please follow the setup guide at https://github.com/ember-cli/eslint-plugin-ember#gtsgjs-eslint-setup',

Use custom parser for gts/gjs #1942

Use custom parser for gts/gjs #1942

Conversation

patricklx commented Aug 22, 2023 • edited Loading

NullVoxPopuli Aug 22, 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

patricklx commented Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patricklx Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patricklx commented Aug 23, 2023 • edited Loading

NullVoxPopuli commented Aug 23, 2023

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

NullVoxPopuli left a comment • edited Loading

Choose a reason for hiding this comment

patricklx commented Aug 24, 2023 • edited Loading

patricklx commented Aug 29, 2023

patricklx commented Oct 19, 2023

NullVoxPopuli commented Oct 19, 2023

patricklx commented Oct 19, 2023

NullVoxPopuli commented Oct 19, 2023

patricklx commented Oct 19, 2023

patricklx commented Oct 19, 2023 • edited Loading

patricklx commented Oct 23, 2023

NullVoxPopuli left a comment

Choose a reason for hiding this comment

NullVoxPopuli commented Oct 31, 2023

patricklx commented Nov 1, 2023

patricklx commented Aug 22, 2023 •

edited

Loading

NullVoxPopuli Aug 22, 2023 •

edited

Loading

patricklx commented Aug 22, 2023 •

edited

Loading

patricklx Aug 23, 2023 •

edited

Loading

patricklx commented Aug 23, 2023 •

edited

Loading

NullVoxPopuli left a comment •

edited

Loading

patricklx commented Aug 24, 2023 •

edited

Loading

patricklx commented Oct 19, 2023 •

edited

Loading