chore(linting): Update versions and avoid {}

[Josh-Walker-GM]

Problem
I was getting a little tired of seeing:

=============

WARNING: You are currently running a version of TypeScript which is not officially supported by @typescript-eslint/typescript-estree.

You may find that it works just fine, or you may not.

SUPPORTED TYPESCRIPT VERSIONS: >=3.3.1 <5.2.0

YOUR TYPESCRIPT VERSION: 5.3.3

Please only submit bug reports when using the officially supported version.

=============

/.../redwood/packages/router/src/skipNav.tsx
  13:17  warning  Don't use `{}` as a type. `{}` actually means "any non-nullish value".
- If you want a type meaning "any object", you probably want `object` instead.
- If you want a type meaning "any value", you probably want `unknown` instead.
- If you want a type meaning "empty object", you probably want `Record<string, never>` instead.
- If you really want a type meaning "any non-nullish value", you probably want `NonNullable<unknown>` instead  @typescript-eslint/ban-types
  13:26  warning  Don't use `{}` as a type. `{}` actually means "any non-nullish value".
- If you want a type meaning "any object", you probably want `object` instead.
- If you want a type meaning "any value", you probably want `unknown` instead.
- If you want a type meaning "empty object", you probably want `Record<string, never>` instead.
- If you really want a type meaning "any non-nullish value", you probably want `NonNullable<unknown>` instead  @typescript-eslint/ban-types
  24:14  warning  Don't use `{}` as a type. `{}` actually means "any non-nullish value".
- If you want a type meaning "any object", you probably want `object` instead.
- If you want a type meaning "any value", you probably want `unknown` instead.
- If you want a type meaning "empty object", you probably want `Record<string, never>` instead.
- If you really want a type meaning "any non-nullish value", you probably want `NonNullable<unknown>` instead  @typescript-eslint/ban-types

✖ 3 problems (0 errors, 3 warnings)

when running yarn lint on the framework. You also see that first WARNING: You are currently running... warning on user apps not just the framework.

Changes

1. Update versions of eslint and the @typescript-eslint packages
2. Switch to vitest for the eslint-plugin package - lets use the same tool everywhere
3. Update some types in packages/router/src/skipNav.tsx to avoid warnings about using {} as a type

Notes
You can find the v6 upgrade guide I followed here: https://typescript-eslint.io/blog/announcing-typescript-eslint-v6/. This covered all the changes needed, namely:

• AST changes
• moduleResolution changes

The v7 upgrade has no implications for us as far as I can tell: https://typescript-eslint.io/blog/announcing-typescript-eslint-v7/

The conversion to vitest was my own choice but seems like a reasonable one.

Outstanding

1. Is this breaking?
2. I picked object over {} but perhaps one of the other possible alternative types is better. I don't know yet but happy to change it if others think there is a better option.
3. Check that I an not an idiot and there isn't a good reason this wasn't done yet.

[Josh-Walker-GM]

Happy to be overruled on the object's here

[jtoar]

Nice to finally get rid of that warning! v7 doesn't look breaking like you said but for v6 it's not immediately clear. It feels like the Vite and Prettier upgrades where they fixed bugs but if a user could somehow configure Vite or Prettier (they can), then technically it's breaking. Here it's ESLint, or TSLint. We don't ship an .eslintrc.js file, we just ship this snippet in a user's package.json:

  "eslintConfig": {
    "extends": "@redwoodjs/eslint-config",
    "root": true
  },

a user is free to add more config here.

It feels harmless but I don't want to deal with the collateral if I'm wrong, so I'd say just make it breaking. Unless you want to deal with the collateral. 😉

There's one thing that feels important to have more clarity on though. It looks like TSLint reorganized their configs: https://typescript-eslint.io/blog/announcing-typescript-eslint-v6/#reworked-configuration-names. E.g., this line may not include the same rules between v5 and v6:

redwood/packages/eslint-config/shared.js

Line 152 in 0e8c0ec

extends: ['plugin:@typescript-eslint/recommended', 'prettier'],

We also may not need to turn off these stylistic rules because they may not be included in the recommended config anymore (if the recommended config still exists; I think they renamed it?):

redwood/packages/eslint-config/shared.js

Lines 153 to 170 in 0e8c0ec

	rules: {
	'@typescript-eslint/no-var-requires': 'off',
	'@typescript-eslint/explicit-function-return-type': 'off',
	'@typescript-eslint/no-empty-interface': 'off',
	'@typescript-eslint/explicit-module-boundary-types': 'off',
	'@typescript-eslint/ban-types': 'warn',
	'no-empty-function': 'off',
	'@typescript-eslint/no-empty-function': 'off',
	camelcase: 'off',
	'@typescript-eslint/camelcase': 'off',
	'no-use-before-define': 'off',
	'@typescript-eslint/no-use-before-define': 'off',
	'@typescript-eslint/prefer-namespace-keyword': 'off',
	'@typescript-eslint/no-unused-vars': [
	'error',
	{ varsIgnorePattern: '^_', argsIgnorePattern: '^_' },
	],
	},

So maybe we should get more insight into that before proceeding. Thoughts?

[Josh-Walker-GM]

Thanks @jtoar. I've added a small script that collates the rules applied to each file and records them, highlighting differences.

I've run that against these changes here and you'll see differences like:

scripts/seed.ts has a removed rule for @typescript-eslint/explicit-function-return-type
scripts/seed.ts has a removed rule for @typescript-eslint/no-empty-interface
scripts/seed.ts has a removed rule for @typescript-eslint/explicit-module-boundary-types
scripts/seed.ts has a removed rule for @typescript-eslint/no-empty-function
scripts/seed.ts has a removed rule for @typescript-eslint/prefer-namespace-keyword
scripts/seed.ts has a new rule for @typescript-eslint/no-duplicate-enum-values
scripts/seed.ts has a new rule for @typescript-eslint/no-unsafe-declaration-merging

My understanding of the reasons for these changes are:

1. @typescript-eslint/explicit-function-return-type doesn't appear to be applied anyway so we don't have to turn it off
2. @typescript-eslint/no-empty-interface stylistic so it is no longer applied and we don't have to turn it off
3. @typescript-eslint/explicit-module-boundary-types doesn't appear to be applied anyway so we don't have to turn it off
4. @typescript-eslint/no-empty-function stylistic so it is no longer applied and we don't have to turn it off
5. @typescript-eslint/prefer-namespace-keyword stylistic so it is no longer applied and we don't have to turn it off. This also appears to be redundant because we do have @typescript-eslint/no-namespace enabled as part of 'recommended' and so that is erroring out where the @typescript-eslint/prefer-namespace-keyword would. Shall we disable this @typescript-eslint/no-namespace now?
6. @typescript-eslint/no-duplicate-enum-values and @typescript-eslint/no-unsafe-declaration-merging appear to be new rules applied under 'recommended' and so I disabled them to keep some parity with what we already have. I think we should just let them be enabled as this will ship in a major anyway.

I agree with what you said and that we should ship this in a major just because it's an underlying major dependency upgrade.

[jtoar]

Amazing!

[jtoar]

Had to put on my imaginary physicist hat for this

[Josh-Walker-GM]

I put my lazy person hat on for that one because I couldn't be bothered waiting for 138 files every time I was testing this haha.

[jtoar]

Approved! There may be one or two changes to clean up in responding to your last comment:

@typescript-eslint/prefer-namespace-keyword stylistic so it is no longer applied and we don't have to turn it off. This also appears to be redundant because we do have @typescript-eslint/no-namespace enabled as part of 'recommended' and so that is erroring out where the @typescript-eslint/prefer-namespace-keyword would. Shall we disable this @typescript-eslint/no-namespace now?

Not sure I'm following you here; you said @typescript-eslint/prefer-namespace-keyword no longer applies, and we were turning it off anyway before. So it wouldn't conflict with @typescript-eslint/prefer-namespace-keyword would it? It looks like we have @typescript-eslint/no-namespace set to error in both the existing and updated configs already.

@typescript-eslint/no-duplicate-enum-values and @typescript-eslint/no-unsafe-declaration-merging appear to be new rules applied under 'recommended' and so I disabled them to keep some parity with what we already have. I think we should just let them be enabled as this will ship in a major anyway.

I'd probably go with the defaults for these like you're saying since this is breaking anyway.

All in all super impressed with the thoroughness. Very nice! 🚀

[Josh-Walker-GM]

Okay updated to allow those new default rules then. I'll fix the conflicts and then merge with the feature-breaking label and the next major milestone.

Yeah sorry for being unclear. We used to disable @typescript-eslint/prefer-namespace-keyword but that rule is somewhat redundant when we have @typescript-eslint/no-namespace enabled which we had both before and after this case. Basically it looks like @typescript-eslint/prefer-namespace-keyword never really mattered that much.