statelyai / xstate

Actor-based state management & orchestration for complex app logic.
https://stately.ai/docs
MIT License
27.21k stars 1.26k forks source link

add eslint #5041

Closed with-heart closed 1 month ago

with-heart commented 3 months ago

This PR installs and configures eslint with typescript-eslint.

It uses the new flat config format (which is such a nice improvement imo), along with the recommended rules from @eslint/js and typescript-eslint.

Since #5022 is a thing, I added all test.* files to the global ignores. Once that lands we can remove this and maybe add eslint-plugin-vitest.

changeset-bot[bot] commented 3 months ago

⚠️ No Changeset found

Latest commit: aee03ce72e0a9d854ee357d581da833a8f70c726

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

with-heart commented 3 months ago

I wanted to try this with type-aware linting but it reported hundreds of errors that didn't seem like they'd be easy to fix. They're likely related to having to set @typescript-eslint/no-explicit-any to warn (581 warnings for that rule alone 😬).

Andarist commented 3 months ago

how many we'd get after disabling @typescript-eslint/no-explicit-any?

with-heart commented 3 months ago

@Andarist 0. Should we disable it? I wasn't sure if this was something that'd be worth trying to deal with or not

with-heart commented 3 months ago

I'm not sure what the deal is with the failed check. The error it reports isn't actually relevant.

Here's the actual error:

This syntax is reserved in files with the .mts or .cts extension. Add a trailing comma, as in `<T,>() => ...`. (20:14)

To make build work, I have to add a trailing comma to type params in packages/xstate-solid:

diff --git a/packages/xstate-solid/src/createImmutable.ts b/packages/xstate-solid/src/createImmutable.ts
index 5624473d5b..e3237c7414 100644
--- a/packages/xstate-solid/src/createImmutable.ts
+++ b/packages/xstate-solid/src/createImmutable.ts
@@ -19,7 +19,7 @@ const updateStore = <Path extends unknown[]>(
   store: Store<any>
 ) => {
   const valueRefs = new WeakMap<any, unknown>();
-  const diff = <CompareValue>(
+  const diff = <CompareValue,>(
     next: CompareValue,
     prev: CompareValue,
     path: Path
diff --git a/packages/xstate-solid/src/deepClone.ts b/packages/xstate-solid/src/deepClone.ts
index e829300845..58bc92e28d 100644
--- a/packages/xstate-solid/src/deepClone.ts
+++ b/packages/xstate-solid/src/deepClone.ts
@@ -17,7 +17,7 @@ export function isWrappable(obj: any): obj is object {
  * @param valueRefs A WeakMap that stores a reference from the original
  *   object/array to the cloned object/array
  */
-const clone = <T>(value: T, valueRefs: WeakMap<any, any>): T => {
+const clone = <T,>(value: T, valueRefs: WeakMap<any, any>): T => {
   if (!isWrappable(value)) {
     return value;
   }
@@ -47,4 +47,4 @@ const clone = <T>(value: T, valueRefs: WeakMap<any, any>): T => {
   return clonedValue;
 };

-export const deepClone = <T>(value: T): T => clone(value, new WeakMap());
+export const deepClone = <T,>(value: T): T => clone(value, new WeakMap());

These previously used extends unknown:

diff --git a/packages/xstate-solid/src/createImmutable.ts b/packages/xstate-solid/src/createImmutable.ts
index 7764371bac..e3237c7414 100644
--- a/packages/xstate-solid/src/createImmutable.ts
+++ b/packages/xstate-solid/src/createImmutable.ts
@@ -19,7 +19,7 @@ const updateStore = <Path extends unknown[]>(
   store: Store<any>
 ) => {
   const valueRefs = new WeakMap<any, unknown>();
-  const diff = <CompareValue extends unknown>(
+  const diff = <CompareValue,>(
     next: CompareValue,
     prev: CompareValue,
     path: Path
diff --git a/packages/xstate-solid/src/deepClone.ts b/packages/xstate-solid/src/deepClone.ts
index 1eeaa76a2d..58bc92e28d 100644
--- a/packages/xstate-solid/src/deepClone.ts
+++ b/packages/xstate-solid/src/deepClone.ts
@@ -17,10 +17,7 @@ export function isWrappable(obj: any): obj is object {
  * @param valueRefs A WeakMap that stores a reference from the original
  *   object/array to the cloned object/array
  */
-const clone = <T extends unknown>(
-  value: T,
-  valueRefs: WeakMap<any, any>
-): T => {
+const clone = <T,>(value: T, valueRefs: WeakMap<any, any>): T => {
   if (!isWrappable(value)) {
     return value;
   }
@@ -50,5 +47,4 @@ const clone = <T extends unknown>(
   return clonedValue;
 };

-export const deepClone = <T extends unknown>(value: T): T =>
-  clone(value, new WeakMap());
+export const deepClone = <T,>(value: T): T => clone(value, new WeakMap());

Not really sure what the issue is here and prettier removes the dangling commas so it's not really a fix.

boneskull commented 2 months ago

I wanted to try this with type-aware linting but it reported hundreds of errors that didn't seem like they'd be easy to fix. They're likely related to having to set @typescript-eslint/no-explicit-any to warn (581 warnings for that rule alone 😬).

IMO this is worth doing. You will probably need to disable stuff if using the recommendedTypeChecked preview; it's pretty strict.

Here's some of the rules I'd suggest tweaking:

// and sometimes you gotta use any
'@typescript-eslint/no-explicit-any': 'off',

// allow ! 
'@typescript-eslint/no-non-null-assertion': 'off',

// allow prefixing unused vars with _
'@typescript-eslint/no-unused-vars': [
  'error',
  {
    argsIgnorePattern: '^_',
    varsIgnorePattern: '^_',
  },
],

// better for overloads
'@typescript-eslint/unified-signatures': [
  'error',
  {
    ignoreDifferentlyNamedParameters: true,
  },
],

Something like the above might get you out of the woods.

Also I'd recommend enabling this (if it's not already enabled by default), which surfaces unused // eslint-disable directives:

{
  linterOptions: {
    reportUnusedDisableDirectives: 'error',
  },
}

I'm not sure what you were using for the parser config, but this is what has worked well for me with tseslint@v8.

languageOptions: {
  parserOptions: {
    projectService: {
      // type-aware linting for non-project config files. modify as necessary
      allowDefaultProject: ['*.js', '.*.js'],
    },
    tsconfigRootDir: import.meta.dirname, // (or __dirname, depending)
  },
},
boneskull commented 2 months ago

I don't think T extends unknown is doing anything in that function, and can just be T.

davidkpiano commented 2 months ago

@with-heart Can you resolve merge conflicts?

with-heart commented 2 months ago

@with-heart Can you resolve merge conflicts?

Done! I think we should be good to go assuming the changes look fine 🙏