gitKrystan / prettier-plugin-ember-template-tag

A prettier plugin for formatting Ember template tags
MIT License
22 stars 12 forks source link

[Bug] doesn't work with eslint<->prettier integration? #20

Closed NullVoxPopuli closed 1 year ago

NullVoxPopuli commented 1 year ago

🐞 Describe the Bug

A clear and concise description of what the bug is.

Ran in to this error:

 pnpm lint:js

> limber@0.0.0 lint:js <repo>/frontend
> eslint . --cache

Oops! Something went wrong! :(

ESLint: 7.32.0

SyntaxError: Invalid regular expression: /\b}> = [__GLIMMER_TEMPLATE(`\b/: Unterminated character class
    at new RegExp (<anonymous>)
    at <repo>/node_modules/.pnpm/eslint-plugin-ember@11.2.0_brav3slrf3bf63fxqqsgu3y6sy/node_modules/eslint-plugin-ember/lib/preprocessors/glimmer.js:120:34
    at Array.map (<anonymous>)
    at mapRange (<repo>/node_modules/.pnpm/eslint-plugin-ember@11.2.0_brav3slrf3bf63fxqqsgu3y6sy/node_modules/eslint-plugin-ember/lib/preprocessors/glimmer.js:106:20)
    at Linter._verifyWithProcessor (<repo>/node_modules/.pnpm/eslint@7.32.0/node_modules/eslint/lib/linter/linter.js:1339:16)
    at Linter._verifyWithConfigArray (<repo>/node_modules/.pnpm/eslint@7.32.0/node_modules/eslint/lib/linter/linter.js:1273:25)
    at Linter.verify (<repo>/node_modules/.pnpm/eslint@7.32.0/node_modules/eslint/lib/linter/linter.js:1235:25)}

🔬 Minimal Reproduction

https://github.com/NullVoxPopuli/limber/pull/545

cd frontend
pnpm i
pnpm lint:js

😕 Actual Behavior

the above error

🤔 Expected Behavior

no error

🌍 Environment

➕ Additional Context

Add any other context about the problem here.

gitKrystan commented 1 year ago

Thanks for the report.

Does running Prettier directly work properly?

gitKrystan commented 1 year ago

I suspect this is because your project already has https://github.com/ember-template-lint/ember-template-lint-plugin-prettier installed, which apparently implements its own undocumented(?) prettier-for-template-tag support. Looking into a solution.

NullVoxPopuli commented 1 year ago

Does running Prettier directly work properly?

it does!

because your project already has

hmmm if I comment out that plugin in my template-lint config, I still get the errors:

gitKrystan commented 1 year ago

It turns out it's not related to the ember-template-lint-plugin-prettier at all.

HOWEVER: That plugin is fully redundant with this one for gjs/gts files. (Except that this one will also format the js portion of your file whereas that one will only format the template portion.)

I am working on a minimal repro currently.

gitKrystan commented 1 year ago

Seems like there should be some escaping happening here: https://github.com/ember-cli/eslint-plugin-ember/blob/db745a3881686d50e486be67d6851d806ac770d6/lib/preprocessors/glimmer.js#L120

It's generating new RegExp('\b[__GLIMMER_\b') which indeed has an opening [, which the RegExp constructor assumes is the beginning of a character set.

~Now going to figure out if/why this plugin causes this to happen in that one.~

Because eslint-plugin-prettier runs Prettier as an ESLint rule, this 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 if that's up to this plugin or the ember-plugin-prettier plugin to fix that yet).

gossi commented 1 year ago

I'm having quite a journey on this one:

And resulted into a debugging session this morning. I think I've come to the root cause of this.

When files processed by eslint, they run through the given parser (most likely @babel/eslint-parser or @typescript-eslint/parser). As they already parse <template> tag into [__GLIMMER_TEMPLATE(...)] to make the code understood to eslint and all the eslint plugins as they operate on plain js. Now, this is also the code that is then passed down into prettier.

Here is my original code:

import Component from '@glimmer/component';

export interface GreetingSignature {
  Element: HTMLSpanElement;
  Args: {
    hello: string;
    to: string;
  };
}

export default class Greeting extends Component<GreetingSignature> {
  get to() {
    return `${this.args.to[0]?.toUpperCase()}${this.args.to.slice(1).toLowerCase()}`;
  }

  <template>
    <span ...attributes>{{@hello}} to {{this.to}}</span>
  </template>
}

which, when it comes to prettier through eslint is looking like so:

import Component from '@glimmer/component';

export interface GreetingSignature {
  Element: HTMLSpanElement;
  Args: {
    hello: string;
    to: string;
  };
}

export default class Greeting extends Component<GreetingSignature> {
  get to() {
    return `${this.args.to[0]?.toUpperCase()}${this.args.to.slice(1).toLowerCase()}`;
  }

  [__GLIMMER_TEMPLATE(`
    <span ...attributes>{{@hello}} to {{this.to}}</span>
  `, { strictMode: true })]
}

which results in this error message:

/Users/thomas/code/gossi/frontend-configs/testing/ember-addon-v2-gts/src/components/greeting.gts
  16:24  error  Replace `⏎····<span·...attributes>{{@hello}}·to·{{this.to}}</span>⏎··` with `<span·...attributes>{{@hello}}·to·{{this.to}}</span>`  prettier/prettier

I would see the problem in the leading and trailing whitespace within the template literal as first param to ___GLIMMER_TEMPLATE(). Perhaps some special treatment here?

NullVoxPopuli commented 1 year ago

I wonder if we need to omit that block surrounded by prettier-disable/prettier-enable directive comments?

gossi commented 1 year ago

I don't think so. As this input:

  [__GLIMMER_TEMPLATE(`
    <span ...attributes>{{@hello}} to {{this.to}}</span>
  `, { strictMode: true })]

when run through pretter turns into:

  [__GLIMMER_TEMPLATE(
    `
    <span ...attributes>{{@hello}} to {{this.to}}</span>
  `,
    { strictMode: true }
  )];

wrapping this in prettier-disable/prettier-enable would keep the [__GLIMMERT_TEMPLATE()] as is. We are interested about the contents within the template literal, which prettier would keep intact.

My assumption here is, that when the parsed code reaches the plugin, it is unsure whether this is trimmed or not (depending on what happened during eslint or during parsing already). Trimming or keeping this by default can be understood as normalization, whichever seems to be the best sensible default for this plugin. If this deviates from the expectations, would might need an extract parameter to control that behavior from the outside.

NullVoxPopuli commented 1 year ago

maybe -- looks like we'd want to debug around here: https://github.com/gitKrystan/prettier-plugin-ember-template-tag/blob/main/src/parse.ts#L162 To see if we're missing information when trying to run prettier through eslint, vs running prettier standalone

gitKrystan commented 1 year ago

@gossi

Based on the prettier/prettier error message you are seeing, I think it's telling you to replace:

  <template>
    <span ...attributes>{{@hello}} to {{this.to}}</span>
  </template>

with

  <template><span ...attributes>{{@hello}} to {{this.to}}</span></template>

Can you confirm that moving the <template> tags to the same line resolves the linter issue? This is, however, unexpected behavior for top-level class templates in the current version of this plugin: https://github.com/gitKrystan/prettier-plugin-ember-template-tag#new-lines Which version are you using?

I suspect this all comes down to https://github.com/ember-cli/eslint-plugin-ember/issues/1659 and that the fix will be more related to how eslint-plugin-ember reports the mismatch.

gossi commented 1 year ago

I'm on the most recent version v0.3.2 (and eslint-plugin-ember@11.5.2)

Yes, exactly as you are describing the issue in the linked https://github.com/ember-cli/eslint-plugin-ember/issues/1659 (wasn't aware about that one).

As you pointed there the \\b${escapeRegExp(token)}\\b regex perhaps needs some special treatment over at eslint-plugin-ember

gitKrystan commented 1 year ago

Hey y'all. I heard a rumor that upgrading to the latest versions of this plugin + eslint-plugin-ember should resolve this issue.

Can you let me know how it goes? If everything looks good, I will close this issue. 🎉

HeroicEric commented 1 year ago

Still seeing this issue using "eslint-plugin-ember: "^11.10.0" & "prettier-plugin-ember-template-tag": "^0.3.2".

Can anyone confirm that it was fixed previously and is just broken again or if it was never actually fixed?

gitKrystan commented 1 year ago

@HeroicEric I responded here: https://github.com/gitKrystan/prettier-plugin-ember-template-tag/issues/81#issuecomment-1650560485

I am marking this issue as fixed, but there is a bug with the fix as mentioned in #81

gitKrystan commented 1 year ago

Closing because this issue is mostly resolved, but there is still a small hiccup as described in #81

jgadbois commented 1 year ago

I just updated to 1.0.0 from 0.3.2 to try to fix the linting errors outlined here with <template> and now it is prompting me to completely delete all content in my .gjs components. (and npm run lint:fix actually did delete all content)

cah-brian-gantzler commented 1 year ago

Same, when prettier runs as part of the linting, entire content of gjs file is deleted.

cah-brian-gantzler commented 1 year ago

After finding the issue https://github.com/gitKrystan/prettier-plugin-ember-template-tag/issues/61, I was using prettier 2.8.8. Updating to prettier 3.0.0 looks like it fixed the issue.

jgadbois commented 1 year ago

@cah-brian-gantzler do you mind sharing the versions of related packages? I upgraded prettier and now getting errors like

Oops! Something went wrong! :(

ESLint: 7.32.0

TypeError: prettier.resolveConfig.sync is not a function
cah-brian-gantzler commented 1 year ago

Tried to include anything that might be relevant. Let me know if you need any others

"@babel/eslint-parser": "^7.22.6",
"ember-template-imports": "^3.4.2",
"ember-template-lint": "^5.11.0",
"eslint": "^8.44.0",
"eslint-config-prettier": "^8.8.0",
"eslint-plugin-ember": "^11.9.0",
"eslint-plugin-node": "^11.1.0",
"eslint-plugin-prettier": "^4.2.1",
"eslint-plugin-qunit": "^7.3.4",
"prettier": "^3.0.0",
"prettier-plugin-ember-template-tag": "^1.0.0",
jgadbois commented 1 year ago

It's just running into more and more errors. I'll have to keep playing with it. I think related to this https://github.com/ember-cli/ember-cli/pull/10142/files and then other prettier related errors are coming into it now.

jgadbois commented 1 year ago

@cah-brian-gantzler I got it all working, thanks for the help.

However, ember-template-lint is still trying to force my components to start on the same line as the initial <template> tag which I thought was part of what this was fixing

So prettier will auto fix

const AgeFilter = <template><Text @type='body-s' @color='secondaryText' @spacing='pt-3'>....</Text></template>

to

const AgeFilter = <template>
   <Text @type='body-s' @color='secondaryText' @spacing='pt-3'>....</Text>
 </template>

But then ember template lint will complain and force the first component to always be on the same line as <template>

Here's current relevent versions in package.json

    "eslint": "^8.44.0",
    "ember-template-lint": "^5.11.0",
    "ember-template-lint-plugin-prettier": "^5.0.0",
    "prettier": "^3.0.0",
    "prettier-plugin-ember-template-tag": "^1.0.0",
    "eslint-config-prettier": "^8.8.0",
    "eslint-plugin-ember": "^11.9.0",
    "eslint-plugin-node": "^11.1.0",
    "eslint-plugin-prettier": "^5.0.0",
    "eslint-plugin-qunit": "^7.2.0",
cah-brian-gantzler commented 1 year ago

The problem I had was prettier was completely blanking the file. Which is fixed.

Just started using template and all mine are local to the class, so dont assign it to a variable. When I get to multiple templates in the same file will let you know what happens

charlesfries commented 1 year ago

@jgadbois I believe there are conflicting or redundant Prettier rules when using prettier-plugin-ember-template-tag and ember-template-lint-plugin-prettier. Try adding this to .template-lintrc.js:

module.exports = {
  ...

  overrides: [
    {
      files: ['**/*.gjs'],
      rules: {
        prettier: false,
      },
    },
  ],
};
jgadbois commented 1 year ago

@charlesfries Thanks - that does remove the errors, but then npm run lint:js and npm run lint:hbs don't do any linting of <template> formatting at all.