Skip to content

feat(eslint-plugin): auto-generate plugin types #7272

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

Closed
wants to merge 1 commit into from

Conversation

bradzacher
Copy link
Member

Background

We want type declarations for our plugin - but turning on type declarations is a PITA for our plugin.

TS wants to transpile each rule file to this .d.ts file:

import type { RuleModule } from '@typescript-eslint/utils/ts-eslint';
declare const _default: RuleModule<TMessageIds, TOptions, TSESLint.RuleListener>;
export default _default;

However because we don't ever do import type { RuleModule } from '@typescript-eslint/utils/ts-eslint'; and we don't often do import type { TSESLint } from '@typescript-eslint/utils'; - TS cannot name the RuleModule type, leading to the following error:

The inferred type of 'default' cannot be named without a reference to
'../../../../node_modules/@typescript-eslint/utils/src/ts-eslint/Rule'.
This is likely not portable. A type annotation is necessary. ts(2742)

In a nutshell TS does not want to automatically add an import to the .d.ts output because it doesn't know if that import might have type side-effects (eg ambient types / global augmentation) - which is completely fair.

In the past we just said "fuck it" and bundled NO types with the plugin at all. But in v6 we had internal usecases for types so we had to add some. We added some manually crafted ones which are okay, but they're obviously quite loose - defining Record<string, T> instead of typing the keys because it saves us trying to keep the keys in sync.

Motivation

ESLint flat configs are coming and they have a new set of needs - namely that users will need to import our plugin to reference in their config - whereas classic configs the reference was implicit and resolved by ESLint.

So we need some external types and we want the keys typed so that users can reference things safely.

Overview

I assessed a few solutions:

  1. keep using manually defined types
    • Didn't like this idea because it's maintenance burden that we don't want
  2. Use tsc with --noEmitOnError=false to force TS to emit despite errors
    • This didn't work because TS doesn't emit .d.ts files for files with TS errors and getting src/rules/index.ts to compile without errors with declarations is not fun.
  3. use @microsoft/api-extractor to generate a type declaration without explicitly using TS
    • Unfortunately the tool ONLY operates on .d.ts files.
  4. generate the types ourselves.
    • This seemed like the best option to automatically keep things in sync without any additional effort!

This PR implements a script to generate the .d.ts files automatically as part of the build step. It's a simple script leveraging our parser to extract the keys and output the following 3 files.

dist/_types/configs.d.ts
import type { Linter } from '@typescript-eslint/utils/ts-eslint';

type Keys =
  | 'all'
  | 'base'
  | 'disable-type-checked'
  | 'eslint-recommended'
  | 'recommended'
  | 'recommended-requiring-type-checking'
  | 'recommended-type-checked'
  | 'strict'
  | 'strict-type-checked'
  | 'stylistic'
  | 'stylistic-type-checked';

export const configs: Readonly<Record<Keys, Linter.Config>>;
dist/_types/index.d.ts
import type { configs } from './configs';
import type { rules } from './rules';

declare const cjsExport: {
  configs: typeof configs;
  rules: typeof rules;
};
export = cjsExport;
dist/_types/rules.d.ts
import type { RuleModule } from '@typescript-eslint/utils/ts-eslint';

type Keys =
  | 'adjacent-overload-signatures'
  | 'array-type'
  | 'await-thenable'
  | 'ban-ts-comment'
  | 'ban-tslint-comment'
  | 'ban-types'
  | 'block-spacing'
  | 'brace-style'
  | 'class-literal-property-style'
  | 'class-methods-use-this'
  | 'comma-dangle'
  | 'comma-spacing'
  | 'consistent-generic-constructors'
  | 'consistent-indexed-object-style'
  | 'consistent-type-assertions'
  | 'consistent-type-definitions'
  | 'consistent-type-exports'
  | 'consistent-type-imports'
  | 'default-param-last'
  | 'dot-notation'
  | 'explicit-function-return-type'
  | 'explicit-member-accessibility'
  | 'explicit-module-boundary-types'
  | 'func-call-spacing'
  | 'indent'
  | 'init-declarations'
  | 'key-spacing'
  | 'keyword-spacing'
  | 'lines-around-comment'
  | 'lines-between-class-members'
  | 'member-delimiter-style'
  | 'member-ordering'
  | 'method-signature-style'
  | 'naming-convention'
  | 'no-array-constructor'
  | 'no-base-to-string'
  | 'no-confusing-non-null-assertion'
  | 'no-confusing-void-expression'
  | 'no-dupe-class-members'
  | 'no-duplicate-enum-values'
  | 'no-duplicate-type-constituents'
  | 'no-dynamic-delete'
  | 'no-empty-function'
  | 'no-empty-interface'
  | 'no-explicit-any'
  | 'no-extra-non-null-assertion'
  | 'no-extra-parens'
  | 'no-extra-semi'
  | 'no-extraneous-class'
  | 'no-floating-promises'
  | 'no-for-in-array'
  | 'no-implied-eval'
  | 'no-import-type-side-effects'
  | 'no-inferrable-types'
  | 'no-invalid-this'
  | 'no-invalid-void-type'
  | 'no-loop-func'
  | 'no-loss-of-precision'
  | 'no-magic-numbers'
  | 'no-meaningless-void-operator'
  | 'no-misused-new'
  | 'no-misused-promises'
  | 'no-mixed-enums'
  | 'no-namespace'
  | 'no-non-null-asserted-nullish-coalescing'
  | 'no-non-null-asserted-optional-chain'
  | 'no-non-null-assertion'
  | 'no-redeclare'
  | 'no-redundant-type-constituents'
  | 'no-require-imports'
  | 'no-restricted-imports'
  | 'no-shadow'
  | 'no-this-alias'
  | 'no-throw-literal'
  | 'no-type-alias'
  | 'no-unnecessary-boolean-literal-compare'
  | 'no-unnecessary-condition'
  | 'no-unnecessary-qualifier'
  | 'no-unnecessary-type-arguments'
  | 'no-unnecessary-type-assertion'
  | 'no-unnecessary-type-constraint'
  | 'no-unsafe-argument'
  | 'no-unsafe-assignment'
  | 'no-unsafe-call'
  | 'no-unsafe-declaration-merging'
  | 'no-unsafe-enum-comparison'
  | 'no-unsafe-member-access'
  | 'no-unsafe-return'
  | 'no-unused-expressions'
  | 'no-unused-vars'
  | 'no-use-before-define'
  | 'no-useless-constructor'
  | 'no-useless-empty-export'
  | 'no-var-requires'
  | 'non-nullable-type-assertion-style'
  | 'object-curly-spacing'
  | 'padding-line-between-statements'
  | 'parameter-properties'
  | 'prefer-as-const'
  | 'prefer-enum-initializers'
  | 'prefer-for-of'
  | 'prefer-function-type'
  | 'prefer-includes'
  | 'prefer-literal-enum-member'
  | 'prefer-namespace-keyword'
  | 'prefer-nullish-coalescing'
  | 'prefer-optional-chain'
  | 'prefer-readonly'
  | 'prefer-readonly-parameter-types'
  | 'prefer-reduce-type-parameter'
  | 'prefer-regexp-exec'
  | 'prefer-return-this-type'
  | 'prefer-string-starts-ends-with'
  | 'prefer-ts-expect-error'
  | 'promise-function-async'
  | 'quotes'
  | 'require-array-sort-compare'
  | 'require-await'
  | 'restrict-plus-operands'
  | 'restrict-template-expressions'
  | 'return-await'
  | 'semi'
  | 'sort-type-constituents'
  | 'space-before-blocks'
  | 'space-before-function-paren'
  | 'space-infix-ops'
  | 'strict-boolean-expressions'
  | 'switch-exhaustiveness-check'
  | 'triple-slash-reference'
  | 'type-annotation-spacing'
  | 'typedef'
  | 'unbound-method'
  | 'unified-signatures';

export const rules: Readonly<
  Record<Keys, RuleModule<string, readonly unknown[]>>
>;

@bradzacher bradzacher added the bug Something isn't working label Jul 19, 2023
@typescript-eslint
Copy link
Contributor

Thanks for the PR, @bradzacher!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

@netlify
Copy link

netlify bot commented Jul 19, 2023

Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit 59f4c29
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/64b7cafeac8fbe000807ff44
😎 Deploy Preview https://deploy-preview-7272--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@bradzacher bradzacher added enhancement New feature or request and removed bug Something isn't working labels Jul 19, 2023
@nx-cloud
Copy link

nx-cloud bot commented Jul 19, 2023

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 59f4c29. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 35 targets

Sent with 💌 from NxCloud.

@codecov
Copy link

codecov bot commented Jul 19, 2023

Codecov Report

Merging #7272 (59f4c29) into main (f2aed1b) will decrease coverage by 0.01%.
The diff coverage is 83.33%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7272      +/-   ##
==========================================
- Coverage   87.40%   87.39%   -0.01%     
==========================================
  Files         381      381              
  Lines       13313    13315       +2     
  Branches     3934     3935       +1     
==========================================
+ Hits        11636    11637       +1     
  Misses       1292     1292              
- Partials      385      386       +1     
Flag Coverage Δ
unittest 87.39% <83.33%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/utils/src/eslint-utils/RuleCreator.ts 70.00% <83.33%> (-5.00%) ⬇️

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for taking a while on this - I kept pausing and coming back assuming I'd missed something. I don't follow why we'd need the simpleTraverse approach rather than a runtime import of built files?

}

const BREAK_TRAVERSE = Symbol();
async function extractExportedKeys(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I read through this and I think it makes sense for the approach of manually traversing through the source files and reading in their declaration nodes... but that's a lot of work. Why not just import from the built output and use the runtime values?

import fs from 'node:fs/promises';
import plugin from '../dist/index.js';

await fs.writeFile(
  './dist/types/rules.d.ts',
  `import type { RuleModule } from '@typescript-eslint/utils/ts-eslint';

type Keys =
${Object.keys(plugin.rules)
  .map(ruleName => `  | '${ruleName}'`)
  .join('\n')}

export const rules: Readonly<
  Record<Keys, RuleModule<string, readonly unknown[]>>
>;
`,
);

(that, but with Prettier formatting, make-dir first, etc.)

const meta = rule?.meta;
if (!meta?.docs) {
if (!rule || !meta?.docs) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: unnecessary change?

Suggested change
if (!rule || !meta?.docs) {
if (!meta?.docs) {

@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Jul 31, 2023
@JoshuaKGoldberg
Copy link
Member

👋 ping @bradzacher - just checking in, is this still something you have time for?

@JoshuaKGoldberg
Copy link
Member

Switching this to draft so it doesn't show up on my queue.

@JoshuaKGoldberg JoshuaKGoldberg marked this pull request as draft January 11, 2024 18:09
@bradzacher
Copy link
Member Author

I'm going to close this for now at least. I thought we'd need this for flat config support but there are ways around this - in #7935 I've essentially imported the plugin with its existing manual types and then add an explicit type to the name within the new package.

In future we might want to add types for rule configs - but for now at least we're not going to.

@bradzacher bradzacher closed this Jan 13, 2024
@bradzacher bradzacher deleted the auto-generate-plugin-types branch January 13, 2024 03:32
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
awaiting response Issues waiting for a reply from the OP or another party enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants