ember-cli / eslint-plugin-ember

An ESLint plugin that provides set of rules for Ember applications based on commonly known good practices.
MIT License
257 stars 198 forks source link

Should we provide a config that disables some rules from `@typescript-eslint/eslint-plugin`? #2110

Open NullVoxPopuli opened 3 months ago

NullVoxPopuli commented 3 months ago

For example, no-use-before-define is often wrong if you define multiple components in the same file.

const Foo = <template><Bar /></template>;
const Bar = <template>hi</template>;

would trigger the error. But this is totally valid, because component rendering is not eager like const-evaluation normally is.

cc @patricklx, in case you have ideas

example in vanilla:

image

patricklx commented 3 months ago

this? https://github.com/ember-cli/eslint-plugin-ember/pull/2107

mmm, this might be different . But it wil be eager later. Because scope is passes directly, right?

NullVoxPopuli commented 3 months ago

mmm, this might be different .

yeah, that PR adds rules to a config, which I don't think we should do.

I imagine the TS config should end up looking like this:

{
  // this is the exact same as what's in the README
  files: ['**/*.gts'],
  parser: 'ember-eslint-parser',
  plugins: ['ember'],
  extends: [
    'eslint:recommended',
    // hopefully this either adds the rules to the config (so that 2107 isn't needed)
    // or we give up on legacy configs and only recommend flat config
    'plugin:@typescript-eslint/recommended',
    'plugin:ember/recommended',
     // this would detect if @typescript-eslint is present, 
     // and make sure no-use-before-define is turned off
    'plugin:ember/recommended-gts', 
  ],
},
patricklx commented 3 months ago

I think the error is correct though. it will be compiled to something like

Foo =  precompileTemplate(`...`,  {
  scope: [Bar]
})
patricklx commented 3 months ago

I think it was pretty clear that they will not change plugin:@typescript-eslint/recommended behaviour. (Lets discuss this part in #2107)

NullVoxPopuli commented 3 months ago

it wilz be compiled to something like

nay, it's

scope: () => ({ Bar })
patricklx commented 3 months ago

it wilz be compiled to something like

nay, it's

scope: () => ({ Bar })

hmm, i took another look, you are right:)

But it looks like the rule isn't looking that exactly anyway: eslint playground

NullVoxPopuli commented 3 months ago

ah! so the eslint rule is wrong!

patricklx commented 3 months ago

I looks like that's actually the behaviour it intended to do:

This rule will warn when it encounters a reference to an identifier that has not yet been declared.

https://eslint.org/docs/latest/rules/no-use-before-define#rule-details

NullVoxPopuli commented 3 months ago

yeah -- but .. it's safe!

patricklx commented 3 months ago

i think this can be closed then? as the rule does work correctly as per its own definition

NullVoxPopuli commented 3 months ago

It works incorrectly for how we need it to tho 🤔