Bug: Created rules using RuleCreator are not portable

[Andarist]

Before You File a Bug Report Please Confirm You Have Done The Following...

• I have tried restarting my IDE and the issue persists.
• I have updated to the latest version of the packages.
• I have searched for related issues and found none that matched my issue.
• I have read the FAQ and my problem is not listed.

Relevant Package

utils

Playground Link

No response

Repro Code

const createRule = ESLintUtils.RuleCreator(name => {
  const ruleName = parsePath(name).name

  return `${REPO_URL}/blob/@emotion/eslint-plugin@${version}/packages/eslint-plugin/docs/rules/${ruleName}.md`
})

// it isn't a problem with `createRule<never[], keyof typeof messages>({`
createRule({
  name: __filename,
  meta: {
    docs: {
      description: 'Ensure vanilla emotion is not used',
      recommended: false
    },
    messages: {
      vanillaEmotion: 'Vanilla emotion should not be used'
    },
    schema: [],
    type: 'problem'
  },
  defaultOptions: [],
  create(context) {
    return {
      ImportDeclaration(node) {
        if (node.source.value === '@emotion/css') {
          context.report({
            node: node.source,
            messageId: 'vanillaEmotion'
          })
        }
      }
    }
  }
})

ESLint Config

No response

tsconfig

No response

Expected Result

A definition file for a library utilizing this util should be generated just fine.

Actual Result

We get an error like this one:

TS2742: The inferred type of 'rules' cannot be named without a reference to '@typescript-eslint/utils/node_modules/@typescript-eslint/types/dist/generated/ast-spec'. This is likely not portable. A type annotation is necessary.

I've diagnosed this and the problem is that the final RuleModule contains a reference to TRuleListener and its type contains references to types from another packages:

typescript-eslint/packages/utils/src/eslint-utils/RuleCreator.ts

Line 95 in ed7b5f6

TRuleListener

I've also figured out a workaround - we need to provide generic params explicitly, to disable the inference for the third generic param (TRuleListener) and just use its default. The workaround in practice can be checked out here: https://github.com/emotion-js/emotion/pull/2761/files/79848d3a5ff7fa5338d31a27fb94951ea15071fa

I wonder though... why the TRuleListener is a generic? Does it have to be referenced in RuleModule? I would have assumed that a rule should be rather opaque to the consumer 🤔 (at least in relation to what kind of AST types does it process and stuff like that)

cc @JoshuaKGoldberg, funny enough that I've stumbled upon this issue just after this recent conversation on Twitter: https://twitter.com/JoshuaKGoldberg/status/1526969335717191692 😅

Additional Info

No response

Versions

package	version
@typescript-eslint/utils	5.25.0
TypeScript	4.5.5
ESLint	^7.10.0

[bradzacher]

As an aside - one thing I would suggest is disabling type def generation for your plugin.

Nobody is going to be importing your plugin directly so giving it type defs is a waste of package size!

[JoshuaKGoldberg]

Hmm, I don't reproduce this when I make a new scratch repo. But when I clone that emotion PR and run yarn build / preconstruct build, I do get the error:

~/repos/emotion$ preconstruct build
🎁 info building bundles!
🎁 error Generating TypeScript declarations for packages/eslint-plugin/src/index.ts failed:
🎁 error packages/eslint-plugin/src/index.ts:8:14 - error TS2742: The inferred type of 'rules' cannot be named without a reference to '@typescript-eslint/utils/node_modules/@typescript-eslint/types/dist/generated/ast-spec'. This is likely not portable. A type annotation is necessary.
🎁 error
🎁 error 8 export const rules = {
🎁 error                ~~~~~
🎁 error
🎁 info If want to learn more about the above error, check https://preconstruct.tools/errors
🎁 info If the error is not there and you want to learn more about it, open an issue at https://github.com/preconstruct/preconstruct/issues/new

I'm not very familiar with rollup. What's the tsc command equivalent it's using under-the-hood? Poking through its cli.cjs.dev.js, it looks like it's spawning a bunch of definition compile tasks?

I wonder though... why the TRuleListener is a generic?

Good question. When we create an override rule such as @typescript-eslint/brace-style that wraps around a core ESLint rule, it was set up to know the base rule's shape.

typescript-eslint/packages/utils/src/ts-eslint/Rule.ts

Line 434 in ed7b5f6

TRuleListener extends RuleListener = RuleListener,

...except, looking through the codebase, none of the createRule calls in override rules use it? Example:

typescript-eslint/packages/eslint-plugin/src/rules/brace-style.ts

Line 15 in ed7b5f6

export default createRule<Options, MessageIds>({

...so, maybe we can get rid of the TRuleListener type parameter? I tried it out locally and it seems fine. cc @bradzacher, I'm low-to-medium confidence here.

[bradzacher]

Probably can get rid of it.
I added it because I just threaded the generics through everywhere, leaving them fore the consumer to use if required.

I doubt anyone does actually use it though! So probably safe to remove.

It is technically a breaking change to remove it from the API, sadly.

[Andarist]

Nobody is going to be importing your plugin directly so giving it type defs is a waste of package size!

That doesn't have to be true - I can imagine people creating presets in a programmatic way and that would "require" type defs to be shipped with the plugin (or at least, it would be better if there would be a type def for a plugin). That being said - I don't have an easy way to opt-out from .d.ts being generated for a single package and I definitely want to generate them for all other packages in the monorepo 😉

I'm not very familiar with rollup. What's the tsc command equivalent it's using under-the-hood? Poking through its cli.cjs.dev.js, it looks like it's spawning a bunch of definition compile tasks?

It's hard to tell what's the exact tsc equivalent because this is generated programmatically through TS APIs, the source code can be found here:
https://github.com/preconstruct/preconstruct/blob/4b4db170a90974f8641ff06678ba4e87bc12c3c1/packages/cli/src/rollup-plugins/typescript-declarations/get-declarations.ts

It is technically a breaking change to remove it from the API, sadly.

I'm in no rush here so that's fine - I just wanted to raise this as an issue. Do you think it's worth raising a PR for this now that would wait for your next major release? Or would you say that this is going to be problematic (merge-wise) and that it's better to wait with such a PR until you actually start working on the new major?

[bradzacher]

I can imagine people creating presets in a programmatic way and that would "require" type defs to be shipped with the plugin

Unless I'm misunderstanding - every reference to an ESLint plugin should be done via raw, untyped strings.
Even programmatically they should be using a string like 'plugin:@typescript-eslint/recommended' to reference your plugin's configs, or '@typescript-eslint/foo': 'error' to reference a rule.

The only case I know of where one might want to directly import a plugin is if you're "extending" a rule (like we do for the base ESLint rules) - in which case that's an advanced edge case I wouldn't ever ship types for!

That being said - I don't have an easy way to opt-out from .d.ts being generated for a single package

😢 that sucks! In our repo we have one tsconfig per package, so we can (and have) disabled typedef generation for the plugin

typescript-eslint/packages/eslint-plugin/tsconfig.build.json

Lines 4 to 6 in 1f60fae

	// specifically disable declarations for the plugin
	"declaration": false,
	"declarationMap": false,

Do you think it's worth raising a PR for this now

More than happy to take a PR now! The code in question rarely changes so it shouldn't be a problem!

[Andarist]

Unless I'm misunderstanding - every reference to an ESLint plugin should be done via raw, untyped strings.
Even programmatically they should be using a string like 'plugin:@typescript-eslint/recommended' to reference your plugin's configs, or '@typescript-eslint/foo': 'error' to reference a rule.

Then that is me not knowing how one actually creates presets in ESLint :P

More than happy to take a PR now! The code in question rarely changes so it shouldn't be a problem!

Here you go :)