refactor: rewrite `createRule` and `RuleTester` #72

SukkaW · 2024-04-04T15:44:25Z

Currently, eslint-plugin-import-x patches @typescript-eslint/utils to add custom properties like the category and modify properties like the recommended.

The PR accomplishes the same thing, by doing some TypeScript exercises instead.

changeset-bot · 2024-04-04T15:44:28Z

⚠️ No Changeset found

Latest commit: 7196d93

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

codesandbox-ci · 2024-04-04T15:44:42Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

codecov · 2024-04-04T16:16:35Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.23%. Comparing base (8b53954) to head (d7fc095).

❗ Current head d7fc095 differs from pull request most recent head 7196d93. Consider uploading reports for the commit 7196d93 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##           master      #72   +/-   ##
=======================================
  Coverage   96.22%   96.23%           
=======================================
  Files          91       91           
  Lines        4399     4409   +10     
  Branches     1497     1498    +1     
=======================================
+ Hits         4233     4243   +10     
  Misses        160      160           
  Partials        6        6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

SukkaW · 2024-04-05T06:58:20Z

@JounQin @silverwind What do you think? Do you prefer creating our own utilities or patching ts-eslint?

SukkaW · 2024-04-05T07:02:56Z

src/utils/create-rule.ts

+  return {
+    ...ESLintUtils.RuleCreator.withoutDocs({
+      meta: metaWithoutDocs,
+      ...rules,
+    }),
+    meta: {
+      ...meta,
+      docs: {
+        ...docs,
+        url: docsUrl(name, commitHash),
+      },
+    },
+  }


Under the hood we are still using ts-eslint's RuleCreator as much as possible, we are only overriding the returned meta property with our meta property.

SukkaW · 2024-04-05T07:05:13Z

test/utils.ts

+      },
+    }
+
+    return super.run(ruleName, modifiedRule, tests)


Under the hood, run$ only does some extra modification before super.run.

Is that possible to use // @ts-expect-error here instead of create a new run$ method?

@JounQin

Adding a ts-expect-error and moving on sounds tempting, but not a best practice here.

TypeScript throws a type mismatch for a reason: Inheritance implies subtyping, and subtype mismatch breaks the Liskov substitution. So TypeScript disallows inheriting from a base class while not being a valid subtype.

The only reason I add ts-expect-error to the constructor is due to the fact that ts-eslint's RuleTester does not extend from the ESLint one; rather, it is an alias of the ESLint's RuleTester, while imposing an unnecessary type (essentially, ts-eslint is declaring an incorrect type definition over other's method), which is why ts-expect-error is applicable there.

Hmm... I think it's some kind of balance choice here, or we can say that the typings in tseslint is incorrect? The modifications are quite huge, aren't they? I'm thinking about the maintenance vs the patch one.

Ideally we can ask eslint-doc-generator and tseslint to support them eachother better instead?

Let's get more feedback from others.

cc @silverwind

Sorry, this is quite far over my head. Only feedback I would have is run$ looks a bit dirty to me. I'd suggest some camelCase name instead.

SukkaW · 2024-04-05T07:06:26Z

test/utils.ts

+  constructor(option: RuleTesterOptions = {}) {
+    /**
+     * The parser option is required in TSESLint.RuleTester, but not in ESLint.RuleTester.
+     * Since TSESLint.RuleTester is just an alias of ESLint.RuleTester, we can safely ignore the type error.
+     */
+    // @ts-expect-error -- parser is not required
+    super(option)
+  }


By extending our own RuleTester class, we don't have to patch the type of the argument.

SukkaW · 2024-04-08T13:22:28Z

@JounQin Would you like to cleat the cache of the GitHub Action? It seems that the CI failed to the corrupt cache.

JounQin · 2024-04-10T14:03:34Z

@SukkaW

I raised typescript-eslint/typescript-eslint#8897 to discuss about the optional parser property.

And I notice patches are also used in tseslint's repository https://github.com/typescript-eslint/typescript-eslint/blob/main/.yarn/patches, and considering there are some // @ts-expect-error - flow type comments in source codes, I'm also considering to add more patches for typings maybe, and the maintenance for such small patches should be easier than TypeScript exercises.

HDYT?

SukkaW · 2024-04-13T15:56:01Z

HDYT?

@JounQin

The debate between patches and self-implementation essentially comes down to which method is easier to maintain.

IMHO, self-implementation is the superior choice. The dist files of @typescript-eslint could undergo substantial changes (e.g. build system change, dist structure change), and in a worst-case scenario, we might find ourselves creating a new patch with each dependency update.

Conversely, existing public APIs tend to offer more stability, providing a solid foundation upon which we can build.

The patch method is only effective for projects that are either no longer actively maintained or are extremely active to the point where patches quickly become obsolete. @typescript-eslint falls into neither of these categories. The response to feature requests and issues from @typescript-eslint is slow, and they frequently alter their build system.

SukkaW added 2 commits April 4, 2024 23:38

refactor: rewrite run and createRule

0c5f2d9

refactor: remove eslint-compat-utils patch

dfc2efd

SukkaW force-pushed the typescript-exercrise branch from 8155716 to a28abc6 Compare April 4, 2024 16:12

refactor: rewrite testrunner

d7fc095

SukkaW force-pushed the typescript-exercrise branch from a28abc6 to d7fc095 Compare April 4, 2024 16:15

chore: add comments

7196d93

SukkaW changed the title ~~refactor: rewrite createRule and RuleTester.run~~ refactor: rewrite createRule and RuleTester Apr 4, 2024

SukkaW commented Apr 5, 2024

View reviewed changes

SukkaW requested a review from JounQin April 7, 2024 08:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: rewrite `createRule` and `RuleTester` #72

refactor: rewrite `createRule` and `RuleTester` #72

SukkaW commented Apr 4, 2024

changeset-bot bot commented Apr 4, 2024 •

edited

codesandbox-ci bot commented Apr 4, 2024 •

edited

codecov bot commented Apr 4, 2024 •

edited

SukkaW commented Apr 5, 2024 •

edited

SukkaW Apr 5, 2024 •

edited

SukkaW Apr 5, 2024 •

edited

JounQin Apr 7, 2024

SukkaW Apr 7, 2024 •

edited

JounQin Apr 8, 2024 •

edited

silverwind Apr 14, 2024

SukkaW Apr 5, 2024

SukkaW commented Apr 8, 2024

JounQin commented Apr 10, 2024

SukkaW commented Apr 13, 2024 •

edited

refactor: rewrite createRule and RuleTester #72

Are you sure you want to change the base?

refactor: rewrite createRule and RuleTester #72

Conversation

SukkaW commented Apr 4, 2024

changeset-bot bot commented Apr 4, 2024 • edited

⚠️ No Changeset found

codesandbox-ci bot commented Apr 4, 2024 • edited

codecov bot commented Apr 4, 2024 • edited

Codecov Report

SukkaW commented Apr 5, 2024 • edited

SukkaW Apr 5, 2024 • edited

Choose a reason for hiding this comment

SukkaW Apr 5, 2024 • edited

Choose a reason for hiding this comment

JounQin Apr 7, 2024

Choose a reason for hiding this comment

SukkaW Apr 7, 2024 • edited

Choose a reason for hiding this comment

JounQin Apr 8, 2024 • edited

Choose a reason for hiding this comment

silverwind Apr 14, 2024

Choose a reason for hiding this comment

SukkaW Apr 5, 2024

Choose a reason for hiding this comment

SukkaW commented Apr 8, 2024

JounQin commented Apr 10, 2024

SukkaW commented Apr 13, 2024 • edited

refactor: rewrite `createRule` and `RuleTester` #72

refactor: rewrite `createRule` and `RuleTester` #72

changeset-bot bot commented Apr 4, 2024 •

edited

codesandbox-ci bot commented Apr 4, 2024 •

edited

codecov bot commented Apr 4, 2024 •

edited

SukkaW commented Apr 5, 2024 •

edited

SukkaW Apr 5, 2024 •

edited

SukkaW Apr 5, 2024 •

edited

SukkaW Apr 7, 2024 •

edited

JounQin Apr 8, 2024 •

edited

SukkaW commented Apr 13, 2024 •

edited