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

Conflicts with prettier-plugin-ember-template-tag + eslint-plugin-prettier when linting gjs/gts files #1659

Closed
gitKrystan opened this issue Nov 8, 2022 · 12 comments · Fixed by #1920 or #1942

Comments

@gitKrystan
Copy link

gitKrystan commented Nov 8, 2022

I had a bug filed on my new project regarding an error being thrown when run via eslint + eslint-plugin-ember + eslint-plugin-prettier: ember-tooling/prettier-plugin-ember-template-tag#20

The further I delve into the issue, the more I think I might need help from this team to solve it, and that possibly the solution might involve changes to this repo rather than mine.

Repro: gitKrystan/simple-gjs-project#2

This is the error thrown:

npm run lint:js                             

> [email protected] lint:js
> eslint . --cache


Oops! Something went wrong! :(

ESLint: 7.32.0

SyntaxError: Invalid regular expression: /\b[__GLIMMER_\b/: Unterminated character class
    at new RegExp (<anonymous>)
    at /Users/krystanhuffmenne/Code/gjs-project/node_modules/eslint-plugin-ember/lib/preprocessors/glimmer.js:121:34
    at Array.map (<anonymous>)
    at mapRange (/Users/krystanhuffmenne/Code/gjs-project/node_modules/eslint-plugin-ember/lib/preprocessors/glimmer.js:106:20)
    at Linter._verifyWithProcessor (/Users/krystanhuffmenne/Code/gjs-project/node_modules/eslint/lib/linter/linter.js:1339:16)
    at Linter._verifyWithConfigArray (/Users/krystanhuffmenne/Code/gjs-project/node_modules/eslint/lib/linter/linter.js:1273:25)
    at Linter.verify (/Users/krystanhuffmenne/Code/gjs-project/node_modules/eslint/lib/linter/linter.js:1235:25)
    at Linter.verifyAndFix (/Users/krystanhuffmenne/Code/gjs-project/node_modules/eslint/lib/linter/linter.js:1428:29)
    at verifyText (/Users/krystanhuffmenne/Code/gjs-project/node_modules/eslint/lib/cli-engine/cli-engine.js:240:48)
    at CLIEngine.executeOnFiles (/Users/krystanhuffmenne/Code/gjs-project/node_modules/eslint/lib/cli-engine/cli-engine.js:808:28)

First of all, it seems like there should be some escaping happening here:

const column = line.search(new RegExp(`\\b${token}\\b`));

Because eslint-plugin-prettier runs Prettier as an ESLint rule, my plugin causes the Prettier ESLint rule to log the following violation message:

{
  ruleId: "prettier/prettier",
  severity: 2,
  message: "Replace `[__GLIMMER_TEMPLATE(`⏎··<h1>···Hello·{{@who}}.·</h1>⏎`,·{·strictMode:·true·})];` with `<template><h1>·Hello·{{@who}}.·</h1></template>`",
  line: 1,
  column: 16,
  nodeType: null,
  messageId: "replace",
  endLine: 3,
  endColumn: 27,
}

Interestingly, the message has the preprocessed [__GLIMMER_TEMPLATE( shenanigans when it probably should not. (Not sure yet if that because of something I am doing or because of something happening in this package).

As a result, the line linked above is generating new RegExp('\b[__GLIMMER_\b') which indeed has an opening [ that the RegExp constructor interprets as the beginning of a character set.

I've noticed that this doesn't always happen. For example, if I use the <template> tag in the "top-level class" position, npm run lint:js works properly (ish..it still has [__GLIMMER_TEMPLATE()] in the message, and I think the line/column numbers might be off):

// works-ish.gjs

import Component from '@glimmer/component';

export default class MyComponent
  extends Component {

        <template>


    <h1>   Hello {{@who}}. </h1>
  </template>
}
npm run lint:js 

> [email protected] lint:js
> eslint . --cache

Debugger attached.

/Users/krystanhuffmenne/Code/gjs-project/app/components/broken.gjs
  2:1  error  Delete `⏎`                                                                                                                                                        prettier/prettier
  2:1  error  Delete `⏎·`                                                                                                                                                       prettier/prettier
  2:1  error  Replace `⏎········[__GLIMMER_TEMPLATE(`⏎⏎⏎····<h1>···Hello·{{@who}}.·</h1>⏎··`,·{·strictMode:·true·})]` with `··<template><h1>·Hello·{{@who}}.·</h1></template>`  prettier/prettier

✖ 3 problems (3 errors, 0 warnings)
@gitKrystan
Copy link
Author

gitKrystan commented Nov 8, 2022

Running the RegExp escape function recommended by MDN on the token before generating the RegExp prevents the error from being thrown, but the line numbers are still way off. I suspect this has to do with the fact that eslint-plugin-ember runs preprocessEmbeddedTemplates before passing the output to Prettier (and my Prettier plugin), which my plugin does not expect. It's very possible this will cause issues w/ formatting as well. Will investigate further tomorrow.

@gitKrystan
Copy link
Author

Here's more info about what is going wrong.

If a user is running the following config:

  • eslint for regular ole’ linting
  • prettier for regular ole’ formatting
  • eslint-plugin-ember for enabling eslint for gjs/gts
  • eslint-plugin-prettier for running prettier via eslint
  • prettier-plugin-ember-template-tag for running prettier on gjs/gts files

This is what happens:

  1. eslint-plugin-ember runs the ember-template-imports preprocesser and passes the preprocessed JS into eslint.
  2. eslint runs as normal using the user’s config on the preprocessed JS.
  3. eslint-plugin-prettier runs prettier on the preprocessed JS.
  4. prettier uses prettier-plugin-ember-template-tag, which assumes the gjs/gts is NOT preprocessed.
    This still "works" in that it formats code, but there is a very good chance it will cause issues with formatting, especially regarding how Prettier handles comments; comments may disappear.
    Additionally, prettier-plugin-ember-template-tag will "un-preprocess" the text when it runs, converting [__GLIMMER_TEMPLATE(...), {}] back to <template>...</template>.
  5. eslint-plugin-prettier compares the output from prettier/prettier-plugin-ember-template-tag (now un-preprocessed) to the input (preprocessed).
    If formatting IS needed, it will report an error with a message like:
    Replace `[__GLIMMER_TEMPLATE(`···<h1>··Hello·World</h1>··`,·{·strictMode:·true·})]` with `<template><h1>·Hello·World</h1></template>`
    
    If formatting is NOT needed, it will STILL report an error due to the un-preprocessing:
    Replace `[__GLIMMER_TEMPLATE(`<h1>Hello·World</h1>`,·{·strictMode:·true·})]` with `<template><h1>Hello·World</h1></template>`
    
  6. eslint forwards this error back to eslint-plugin-ember.
  7. eslint-plugin-ember attempts to map the errors back to the original un-preprocessed text.

By the end everything is all sort of messed up, which causes bugs like:

  • Syntax errors due to eslint-plugin-ember treating "[__GLIMMER_TEMPLATE" as a token and trying to use it in a RegExp. (We can probably fix this w/ a simple escape function as mentioned above.)
  • Line numbers and columns are way off.
  • eslint-plugin-prettier ALWAYS reports an error, regardless of if the template needs formatting.
  • The errors contain confusing stuff about [__GLIMMER_TEMPLATE(...), {}] which will be nonsensical to the user.

@bmish
Copy link
Member

bmish commented Nov 10, 2022

@NullVoxPopuli @nlfurniss?

@nlfurniss
Copy link
Contributor

prettier uses prettier-plugin-ember-template-tag, which assumes the gjs/gts is NOT preprocessed.

Is the issue that the input from eslint comes preprocesses, and then prettier-plugin-ember-template-tag "preprocesses" it again? And if so, can the prettier plugin detect whether the input is already preprocessed?

@gitKrystan
Copy link
Author

gitKrystan commented Nov 14, 2022

Re-preprocessing is essentially no-op, so that won't fix the issues unfortunately. However, I do think that detecting whether the input is already preprocessed might be useful.

I suspect there are multiple issues happening here but the main issues appear to be:

  • False positives:
    prettier-plugin-ember-template-tag assumes that it is going to receive gjs/gts (<template>...</template>) and be responsible for preprocessing. Instead it is receiving js/ts ([__GLIMMER_TEMPLATE(...)]) since eslint-plugin-ember is also preprocessing. Then it spits out gjs/gts (<template>...</template>), which eslint-plugin-prettier then diffs with the preprocessed js/ts ([__GLIMMER_TEMPLATE(...)]). This will always be a diff even if the user's gjs/gts was formatted perfectly to begin with.
  • Confusing output (which coincidentally results in syntax errors):
    Even once we fix the above, we still have an issue when the user's gjs/gts does need to be formatted. Because eslint-plugin-prettier is receiving [__GLIMMER_TEMPLATE(...)] and seeing that the output should be <template>...</template>, the generated eslint error exposes the internal [__GLIMMER_TEMPLATE(...)] thing that the average user shouldn't need to know anything about.
    Replace `[__GLIMMER_TEMPLATE(`···<h1>··Hello·World</h1>··`,·{·strictMode:·true·})]` with `<template><h1>·Hello·World</h1></template>`
    
    Coincidentally, this also causes a syntax error w/ the constructed RegExp that eslint-plugin-ember uses to attach errors to their associated lines/columns.
  • Incorrect line numbers:
    In some cases the syntax error does not happen but it looks like the token being used by eslint-plugin-ember to attach errors to their associated lines/columns in these cases is just "" so it's always matching with line 1 column 1.

I am working on a version of prettier-plugin-ember-template-tag that detects preprocessed input and spits out formatted code that still contains [__GLIMMER_TEMPLATE(...)] with the assumption that whatever library passed [__GLIMMER_TEMPLATE(...)] to prettier.format(...) knows what it's doing and will convert that output back to <template>...</template> at the appropriate time. Hopefully eslint-plugin-ember can be modified to be compatible with this version. I'm poking around to see what changes could be made to do that.

@NullVoxPopuli
Copy link
Contributor

@gitKrystan sounds awesome!
once you have that work released, I'll get going on the fixes for eslint-plugin-ember

@gitKrystan
Copy link
Author

@NullVoxPopuli I just released prettier-plugin-ember-template-tag 0.1.0 which should have all the fixes necessary. I also did a little spike to make sure the fix would actually be helpful for this library:

https://github.com/ember-cli/eslint-plugin-ember/compare/master...gitKrystan:gjs-prettier?expand=1

The tests are still failing, but things are way closer than they were before.

@mfeckie
Copy link

mfeckie commented May 17, 2023

Any progress with this?

@NullVoxPopuli
Copy link
Contributor

Eslint plugin ember fixed fixing yesterday - is this issue still an issue after updating?

@backspace
Copy link

It worked for me 🎉 I was getting Prettier errors in preprocessed gts files and after updating eslint-plugin-ember to 11.6.0 they’re gone (other lint errors remain.

@gitKrystan
Copy link
Author

I think this issue is mostly fixed, but there is still a bug when running prettier on render(<template>...</template>) in test files:
ember-tooling/prettier-plugin-ember-template-tag#81

@bmish
Copy link
Member

bmish commented Aug 22, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants