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

The one lint config to rule them all (also how to get gjs/gts linting going) #71

Closed
NullVoxPopuli opened this issue Oct 28, 2022 · 7 comments

Comments

@NullVoxPopuli
Copy link
Collaborator

NullVoxPopuli commented Oct 28, 2022

A proposal in brief:

Covers:

  • app
  • addon
  • v2 addon
  • JS / gjs
  • TS / gts
  • all node files added to the root directory of any project
  • easy to change
  • conditional typescript -- based on the inclusion of typsecript as a resolveable module -- cc @chriskrycho
  • I want to recommend this for the default blueprint for apps/addons in ember-cli as well

Thoughts?:

sample repo if cloning is easier: https://github.com/NullVoxPopuli/ember-lint-demonstrate-overrides-based-config

But also trying this out on a real project: NullVoxPopuli/limber#545

'use strict';

function hasDep(depName) {
  try {
    return Boolean(require.resolve(depName));
  } catch (e) {
    if (e.message.startsWith(`Cannot find module '${depName}'`)) return false;

    throw e;
  }
}

function proposedEmberDefault(personalPreferences) {
  let hasTypeScript = hasDep('typescript');

  const configBuilder = {
    modules: {
      browser: {
        get js() {
          return {
            files: [
              '{src,app,addon,addon-test-support,tests}/**/*.{gjs,js}',
              'tests/dummy/config/deprecation-workflow.js',
              'config/deprecation-workflow.js',
            ],
            parser: 'babel-eslint',
            parserOptions: {
              ecmaVersion: 2022,
              sourceType: 'module',
              ecmaFeatures: {
                legacyDecorators: true,
              },
            },
            plugins: ['ember'],
            extends: [
              'eslint:recommended',
              'plugin:ember/recommended',
              'plugin:prettier/recommended',
            ],
            env: {
              browser: true,
            },
            rules: {
              ...personalPreferences.rules,
            },
          }
        },
        get ts() {
          if (!hasTypeScript) return;

          return {
            files: ['{src,app,addon,addon-test-support,tests,types}/**/*.{gts,ts}'],
            parser: '@typescript-eslint/parser',
            plugins: ['ember'],
            extends: [
              'eslint:recommended',
              'plugin:ember/recommended',
              'plugin:prettier/recommended',
              'plugin:@typescript-eslint/recommended',
            ],
            env: {
              browser: true,
            },
            rules: {
              // type imports are removed in builds
              '@typescript-eslint/consistent-type-imports': 'error',

              // prefer inference, but it is recommended to declare
              // return types around public API
              '@typescript-eslint/explicit-function-return-type': 'off',
              '@typescript-eslint/explicit-module-boundary-types': 'off',

              // Allows placeholder args to still be defined for
              // documentation or "for later" purposes
              '@typescript-eslint/no-unused-vars': ['error', { argsIgnorePattern: '^_' }],
              ...personalPreferences.rules,
            },
          }
        },
        get declarations() {
          if (!hasTypeScript) return;

          return {
            files: ['**/*.d.ts'],
            extends: [
              'eslint:recommended',
              'plugin:prettier/recommended',
              'plugin:@typescript-eslint/recommended',
            ],
            env: {
              browser: true,
            },
            rules: {
              '@typescript-eslint/no-empty-interface': 'off'
            }
          }
        }
      },
      tests: {
        get js() {
          let browserJS = configBuilder.modules.browser.js;
          return {
            ...browserJS,
            files: ['tests/**/*-test.{gjs,js}'],
            extends: [...browserJS.extends, 'plugin:qunit/recommended'],
          }
        },
        get ts() {
          if (!hasTypeScript) return;

          let browserTS = configBuilder.modules.browser.ts;

          return {
            ...browserTS,
            files: ['tests/**/*-test.{gts,ts}'],
            extends: [...browserTS.extends, 'plugin:qunit/recommended'],
          }
        },
      }
    },
    commonjs: {
      node: {
        get js() {
          return {
            files: [
              './*.{cjs,js}',
              './config/**/*.js',
              './lib/*/index.js',
              './server/**/*.js',
              './blueprints/*/index.js',
            ],
            parserOptions: {
              sourceType: 'script',
            },
            env: {
              browser: false,
              node: true,
            },
            plugins: ['node'],
            extends: ['plugin:node/recommended'],
            rules: {
              ...personalPreferences.rules,
              // this can be removed once the following is fixed
              // https://github.com/mysticatea/eslint-plugin-node/issues/77
              'node/no-unpublished-require': 'off',
            },
          }
        }
      }
    }
  }

  return configBuilder;
}


const personalPreferences = {
  rules: {
    // const has misleading safety implications
    // look in to "liberal let"
    'prefer-const': 'off',

    // people should know that no return is undefined in JS
    'getter-return': ['error', { allowImplicit: true }],

    'padding-line-between-statements': [
      'error',
      { blankLine: 'always', prev: '*', next: 'return' },
      { blankLine: 'always', prev: '*', next: 'break' },
      { blankLine: 'always', prev: '*', next: 'block-like' },
      { blankLine: 'always', prev: 'block-like', next: '*' },
      { blankLine: 'always', prev: ['const', 'let'], next: '*' },
      { blankLine: 'always', prev: '*', next: ['const', 'let'] },
      { blankLine: 'any', prev: ['const', 'let'], next: ['const', 'let'] },
      { blankLine: 'any', prev: ['*'], next: ['case'] },
    ],
  },
}

const config = proposedEmberDefault(personalPreferences);

module.exports = {
  root: true,
  /**
   * No root rules needed, because we define everything with overrides
   * so that understanding what set of rules is applied to what files
   * is easier to understand.
   *
   * This can be debugged with
   *
   * eslint --print-config ./path/to/file
   */
  rules: {},
  overrides: [
    config.commonjs.node.js,
    config.modules.browser.js,
    config.modules.browser.ts,
    config.modules.browser.declarations,
    config.modules.tests.js,
    config.modules.tests.ts,
  ].filter(Boolean),
};
@NullVoxPopuli
Copy link
Collaborator Author

@NullVoxPopuli
Copy link
Collaborator Author

cc @bendemboski as well

@NullVoxPopuli
Copy link
Collaborator Author

since it's important to know how a file works when you first open it, we could move the actual .eslint config to the top of the file using using a getter within the main config:

'use strict';

module.exports = {
  root: true,
  /**
   * No root rules needed, because we define everything with overrides
   * so that understanding what set of rules is applied to what files
   * is easier to understand.
   *
   * This can be debugged with
   *
   * eslint --print-config ./path/to/file
   */
  rules: {},
  get overrides() {
    const config = proposedEmberDefault(personalPreferences);

    return [
      config.commonjs.node.js,
      config.modules.browser.js,
      config.modules.browser.ts,
      config.modules.browser.declarations,
      config.modules.tests.js,
      config.modules.tests.ts,
    ].filter(Boolean);
  }
};

// ... everything else in the file below here

@NullVoxPopuli
Copy link
Collaborator Author

NullVoxPopuli commented Dec 8, 2022

Proposed comments to be generated with the config file:

'use strict';

/**
  * Demonstration of potential default Lint configuration for Ember.JS
  * Supporting this matrix:
  *  - app, addon, tests, v2 addon
  *  - javascript, typescript, typescript declarations
  *
  * This first part is the minimal config provided to ESLint,
  * represented in terms of "overrides"-only, which is much easier
  * to understand than ESLint's traditional "cascading configs".
  *
  * See: https://eslint.org/blog/2022/08/new-config-system-part-2/
  *
  * Related: https://github.com/eslint/eslint/discussions/16557
  */
module.exports = {
  root: true,
  /**
   * No root rules needed, because we define everything with overrides
   * so that understanding what set of rules is applied to what files
   * is easier to understand.
   *
   * This can be debugged with
   *
   * eslint --print-config ./path/to/file
   */
  rules: {},

  /**
  * Getters are used so that we can dynamically add-in various plugins
  * based on the consumer's scenario.
  *
  * For example:
  *  - only include TypeScript rules if `typescript` is present in the
  *    hosting project.
  */
  get overrides() {
    /**
     * Personal preferences may be passed to the config generator.
     * It's only a little helpful utility for deep-merging some things into the browser-
     * based config objects.
     * This is because, when managing an eslint config, this is what folks will care about first.
     * Overriding defaults.
     *
     * This is for integrating with with all the variants, the easy way.
     * Obviously, if folks want to extend each of the `config.{format}.{platform}.{type}`
     * objects above, the ESLint overrides API is all public, and doing so is straight forward.
     */
    const config = proposedEmberDefault(personalPreferences);

    return [
      /**
       * Each of these can be overriden via
       * {
       * ...config.commonjs.node.js,
       *   rules: {
       *   ...config.commonjs.node.js.rules,
       *    'my-custom-rule-here': 'error',
       *   }
       * }
       */
      config.commonjs.node.js,
      config.modules.browser.js,
      config.modules.browser.ts,
      config.modules.browser.declarations,
      config.modules.tests.js,
      config.modules.tests.ts,
    ].filter(Boolean);
  }
};

@NullVoxPopuli
Copy link
Collaborator Author

It turns out that the latest eslint config documentation recommends that we do exactly what I'm proposing here! yay!!
https://eslint.org/docs/latest/use/configure/configuration-files-new#using-predefined-configurations

cc @bmish 🎉

@NullVoxPopuli
Copy link
Collaborator Author

NullVoxPopuli commented Aug 19, 2023

gonna close this as it's really a meta issue across all ember projects, not addon-blueprint specifically -- also next time I propose a lint config change, I want it to use ESLint9's format (which is very nearly the above)

@NullVoxPopuli
Copy link
Collaborator Author

Note, the details of this have changed a bit since we now have a real parser for eslint: https://github.com/ember-cli/eslint-plugin-ember?tab=readme-ov-file#gtsgjs

but the strategy is the same, and implemented over here: https://github.com/NullVoxPopuli/eslint-configs/blob/main/configs/ember.js#L41-L43

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

No branches or pull requests

1 participant