ember-cli / eslint-plugin-ember

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

new "flat" configs contain invalid parser key #2061

Closed Techn1x closed 7 months ago

Techn1x commented 8 months ago

This makes them not usable for anyone trying to switch to flat eslint config

Each of the configs import the "base" config

https://github.com/ember-cli/eslint-plugin-ember/blob/e6d28027d06f2d71fac068c3503dc9cb0f134497/lib/config/recommended-gts.js#L4

But that "base" config contains "parser" key, which is invalid in a flat config - needs to be inside languageOptions, and import the parser rather than providing a string value

https://github.com/ember-cli/eslint-plugin-ember/blob/e6d28027d06f2d71fac068c3503dc9cb0f134497/lib/config/base.js#L14

bmish commented 8 months ago

@NullVoxPopuli @patricklx can you look into this? I've confirmed it only breaks when gjs/gts files are present.

This partially fixes it:

const emberEslintParser = require('ember-eslint-parser');
//...
{ 
  languageOptions: {
    parser: emberEslintParser,
  },
}

But then we get:

TypeError: Expected string in the form "pluginName/objectName" but found "ember/<noop>".

Possible because <noop> may not be considered a valid processor name? (should it be?)

https://github.com/eslint/eslint/blob/1f3744278433006042b8d5f4e9e1e488b2bbb011/lib/config/flat-config-schema.js#L217

Techn1x commented 8 months ago

TypeError: Expected string in the form "pluginName/objectName" but found "ember/<noop>".

Possible because <noop> may not be considered a valid processor name? (should it be?)

Seems likely the reason.

If it's difficult to rename that processor for some reason, or we want to remove it as an issue entirely - under a flat config you can also import and use an object as the processor here, instead of the string name

https://eslint.org/blog/2022/08/new-config-system-part-2/#processors-works-in-a-similar-way-to-eslintrc

The one addition in flat config is that processor can now also be an object containing both a preprocess() and a postprocess() method.

Techn1x commented 8 months ago

Possible because may not be considered a valid processor name? (should it be?)

some background https://github.com/eslint/eslint/pull/14321#discussion_r615544005 https://github.com/eslint/eslint/pull/14321#discussion_r622393611

NullVoxPopuli commented 8 months ago

i wonder if both preprocessors should just be references, rather than strings

bmish commented 8 months ago

@patricklx can you comment about the <noop> parser name and intention with that? Seems like it shouldn't be too difficult to fix this for flat config.

NullVoxPopuli commented 8 months ago

@Techn1x can you provide the config you're using?

Techn1x commented 8 months ago

Unfortunately I couldn't fully switch because typescript/eslint also do not have a flat config yet (and also this issue here in eslint-plugin-ember)

So I settled for just using overrides key in the meantime, in order to be easier to switch when the day comes. If it helps, here it is

// .eslintrc.js
const commonEmberRules = {
  'ember/route-path-style': 'error',
  // ember rules to work on
  'ember/no-get': 'off', // remaining usage is limited & mostly necessary. Too dangerous as warn due to autofixing.
  'ember/no-computed-properties-in-native-classes': 'warn',
  'ember/no-classic-components': 'warn',
  'ember/no-runloop': 'warn',
  'ember/no-array-prototype-extensions': 'warn',
  // off to avoid some lint noise for a while..
  'ember/require-tagless-components': 'off',
  'ember/no-classic-classes': 'off',
}

const commonTypescriptRules = {
  'no-unused-vars': 'off',
  '@typescript-eslint/no-unused-vars': [
    'error',
    {
      vars: 'all',
      args: 'after-used',
      argsIgnorePattern: '^_',
      destructuredArrayIgnorePattern: '^_',
      ignoreRestSiblings: true,
    },
  ],
  '@typescript-eslint/no-empty-function': 'off',
  '@typescript-eslint/consistent-type-imports': 'error',
  '@typescript-eslint/no-explicit-any': 'warn',
}

module.exports = {
  root: true,
  overrides: [
    // JS / TS
    {
      files: ['**/*.{js,ts}'],
      plugins: ['ember', '@typescript-eslint'],
      parser: '@typescript-eslint/parser',
      extends: [
        'eslint:recommended',
        'plugin:ember/recommended',
        'plugin:@typescript-eslint/recommended',
        // From eslint-config-prettier - this disables stylistic rules that prettier handles. should be last.
        'prettier',
      ],
      env: {
        browser: true,
      },
      rules: {
        ...commonTypescriptRules,
        ...commonEmberRules,
      },
    },
    // GTS
    {
      files: ['**/*.gts'],
      plugins: ['ember'],
      parser: 'ember-eslint-parser',
      extends: [
        'eslint:recommended',
        'plugin:ember/recommended',
        'plugin:ember/recommended-gts',
        // From eslint-config-prettier - this disables stylistic rules that prettier handles. should be last.
        'prettier',
      ],
      env: {
        browser: true,
      },
      rules: {
        ...commonTypescriptRules,
        ...commonEmberRules,
      },
      globals: {
        // give glint/gts files access to global types
        ModelFor: 'readonly',
        Nullable: 'readonly',
        StudentEventsService: 'readonly',
      },
    },
    // GJS
    {
      files: ['**/*.gjs'],
      plugins: ['ember'],
      parser: 'ember-eslint-parser',
      extends: [
        'eslint:recommended',
        'plugin:ember/recommended',
        'plugin:ember/recommended-gjs',
        // From eslint-config-prettier - this disables stylistic rules that prettier handles. should be last.
        'prettier',
      ],
      env: {
        browser: true,
      },
      rules: {
        ...commonEmberRules,
      },
    },
    // co-located tests
    {
      files: ['app/**/*-test.{js,ts,gjs,gts}'],
      rules: {
        // This rule does not support co-located tests
        'ember/no-test-support-import': 'off',
      },
    },
    // mirage uses some functions that look like array prototype extensions
    {
      files: ['mirage/**/*.{js,ts}'],
      rules: {
        'ember/no-array-prototype-extensions': 'off',
      },
    },
    // node files
    {
      files: [
        './.eslintrc.js',
        './.prettierrc.js',
        './.template-lintrc.js',
        './ember-cli-build.js',
        './testem.js',
        './blueprints/*/index.js',
        './config/**/*.js',
        './lib/*/index.js',
        './server/**/*.js',
        'tailwind.config.js',
        './contentful/**/*.js',
      ],
      extends: ['eslint:recommended', 'plugin:n/recommended'],
      parserOptions: {
        sourceType: 'script',
      },
      env: {
        browser: false,
        node: true,
      },
      rules: {
        'n/no-unpublished-require': 'off',
        '@typescript-eslint/no-var-requires': 'off',
      },
    },
  ],
}
NullVoxPopuli commented 8 months ago

I got this far with trying to create a test project:

But ran in to:

And downgrading the parser didn't seem to fix the problem, which is a bit disappointing. :(

Techn1x commented 8 months ago

And downgrading the parser didn't seem to fix the problem, which is a bit disappointing. :(

Sad. They did merge a fix though, hopefully they cut the next patch release shortly

NullVoxPopuli commented 7 months ago

Here is an example flat config: https://github.com/NullVoxPopuli/ember-eslint-parser/blob/main/test-projects/configs/flat-js/eslint.config.mjs

Currently, it only does GJS (I'll PR one that works on GTS later)

bmish commented 7 months ago

@patricklx can you comment about the <noop> parser name and intention with that? Seems like it shouldn't be too difficult to fix this for flat config.

@patricklx ping on this? Shall we just rename it to fix this issue?

patricklx commented 7 months ago

Hi, sure can be renamed to anything, just need to make sure to keep update the config, legacy as well

bmish commented 7 months ago

What do you think it should be renamed to? This processor name <noop> is part of the public API but since it's not a valid name, we need to change it.

NullVoxPopuli commented 7 months ago

what's the issue? (am I confused? lol)

This is what a flat config looks like: https://github.com/NullVoxPopuli/ember-eslint-parser/blob/main/test-projects/configs/flat-js/eslint.config.mjs

patricklx commented 7 months ago

Well, the noop processor is only there to check if the setup is correct. It's not really required

Techn1x commented 7 months ago

Just throwing in my uninformed comments. Sorry if this is too many cooks in the kitchen 😄

Unfortunately I couldn't fully switch because typescript/eslint also do not have a flat config yet

They've since cut a release with a flat config - going to attempt a flat config in my project today.

I got this far with trying to create a test project: ... But ran in to: [babel legacy decorators bug] https://github.com/babel/babel/issues/16239

Looks like they have merged a fix and cut a release for that now, hopefully no longer a blocker

what's the issue? (am I confused? lol)

Not sure who this is directed at - but to clarify - I could write a flat config for my project as you have done over in ember-eslint-parser where you mostly just imported the recommended rules from this project (eslint-plugin-ember) and defined the rest yourself. But the README for this project (eslint-plugin-ember) suggests that the recommended config from this project should be usable as a flat config, when it currently can't be due to it not actually being flat-config-compatible.

https://github.com/ember-cli/eslint-plugin-ember/blob/181a53eb2c3720e491e0dc0424720ac6b155b2cf/README.md?plain=1#L28-L37

Techn1x commented 7 months ago

If it's helpful, this is what I ended up with as flat config for my project - doesn't use the configs from this project, decided to define everything myself

Some extra notes. Until eslint v9 is released, flat config and mjs/cjs files aren't on by default. So when running in CLI need to use a command like; ESLINT_USE_FLAT_CONFIG=true eslint . --quiet --config eslint.config.mjs And also need to configure .vscode/settings.json

  "eslint.experimental.useFlatConfig": true,
  "eslint.options": {
    "overrideConfigFile": "eslint.config.mjs"
  },
expand for config ```mjs import globals from 'globals' import js from '@eslint/js' import tseslint from 'typescript-eslint' import eslintPluginEmber from 'eslint-plugin-ember' import eslintPluginNode from 'eslint-plugin-n' import emberParser from 'ember-eslint-parser' import babelParser from '@babel/eslint-parser' import emberConfig from 'eslint-plugin-ember/configs/recommended' import emberGJSConfig from 'eslint-plugin-ember/configs/recommended-gjs' import emberGTSConfig from 'eslint-plugin-ember/configs/recommended-gts' import prettierConfig from 'eslint-config-prettier' const eslintPluginTypescript = tseslint.plugin const typescriptParser = tseslint.parser /** * typescript-eslint doesn't seem to export a useful flat set of rules for a recommended set, rather full configs. * So we build the rules we need out of those configs. */ const typescriptRules = { ...tseslint.configs.recommended.reduce((rules, item) => ({ ...rules, ...item.rules }), {}), } const commonEmberRules = { 'ember/route-path-style': 'error', // ember rules to work on 'ember/no-get': 'off', // remaining usage is limited & mostly necessary. Too dangerous as warn due to autofixing. 'ember/no-computed-properties-in-native-classes': 'warn', 'ember/no-classic-components': 'warn', 'ember/no-runloop': 'warn', 'ember/no-array-prototype-extensions': 'warn', // off to avoid some lint noise for a while.. 'ember/require-tagless-components': 'off', 'ember/no-classic-classes': 'off', } const commonJavascriptRules = { 'no-unused-vars': [ 'error', { vars: 'all', args: 'after-used', argsIgnorePattern: '^_', destructuredArrayIgnorePattern: '^_', ignoreRestSiblings: true, }, ], } const commonTypescriptRules = { ...typescriptRules, // replace the JS rule with a TS one 'no-unused-vars': 'off', '@typescript-eslint/no-unused-vars': [ 'error', { vars: 'all', args: 'after-used', argsIgnorePattern: '^_', destructuredArrayIgnorePattern: '^_', ignoreRestSiblings: true, }, ], '@typescript-eslint/no-empty-function': 'off', '@typescript-eslint/consistent-type-imports': 'error', '@typescript-eslint/no-explicit-any': 'warn', } export default [ js.configs.recommended, // standard eslint recommended config // JS { files: ['**/*.js'], plugins: { ember: eslintPluginEmber, }, languageOptions: { parser: babelParser, parserOptions: { sourceType: 'module', requireConfigFile: false, babelOptions: { plugins: [['@babel/plugin-proposal-decorators', { decoratorsBeforeExport: true }]], }, }, globals: { ...globals.browser, }, }, rules: { ...emberConfig.rules, ...commonEmberRules, ...commonJavascriptRules, }, }, // TS { files: ['**/*.ts'], plugins: { ember: eslintPluginEmber, '@typescript-eslint': eslintPluginTypescript, }, languageOptions: { parser: typescriptParser, globals: { ...globals.browser, }, }, rules: { ...emberConfig.rules, ...commonEmberRules, ...commonTypescriptRules, }, }, // GJS { files: ['**/*.gjs'], plugins: { ember: eslintPluginEmber, }, languageOptions: { parser: emberParser, globals: { ...globals.browser, }, }, rules: { ...emberGJSConfig.rules, ...commonEmberRules, ...commonJavascriptRules, }, }, // GTS { files: ['**/*.gts'], plugins: { ember: eslintPluginEmber, '@typescript-eslint': eslintPluginTypescript, }, languageOptions: { parser: emberParser, globals: { ...globals.browser, }, }, rules: { ...emberGTSConfig.rules, ...commonEmberRules, ...commonTypescriptRules, }, }, // mirage uses some functions that look like array prototype extensions { files: ['tests/mirage/**/*.{js,ts}'], rules: { 'ember/no-array-prototype-extensions': 'off', }, }, // node files { ...eslintPluginNode.configs['flat/recommended-script'], files: [ '.prettierrc.js', '.template-lintrc.js', 'ember-cli-build.js', 'testem.js', 'blueprints/*/index.js', 'config/**/*.js', 'lib/*/index.js', 'server/**/*.js', 'tailwind.config.js', 'contentful/**/*.js', ], rules: { ...eslintPluginNode.configs['flat/recommended-script'].rules, 'n/no-unpublished-require': 'off', '@typescript-eslint/no-var-requires': 'off', }, }, // this disables stylistic rules that prettier handles. should be last. prettierConfig, { // equivalent of old .eslintignore file ignores: [ // compiled output 'dist', 'tmp', // misc '!.*', '.*/', '.eslintcache', 'node_modules', ], }, ] ```
bmish commented 7 months ago

I think this should fix it, please test if you can. The issue with our flat config is described in this issue and I have reproduced it to confirm it's a problem.

Techn1x commented 7 months ago

If it's helpful, this is what I ended up with as flat config for my project

In case anyone's using that as an exmaple, a small refinement to that config to use tseslint.config() function can be found here; https://github.com/typescript-eslint/typescript-eslint/issues/8465#issuecomment-1945287707