Skip to content
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

feat(#16): update typescript-eslint dependencies #17

Closed

Conversation

JH1ller
Copy link

@JH1ller JH1ller commented Mar 27, 2024

closes #16

@karlhorky
Copy link
Contributor

karlhorky commented Apr 8, 2024

@hluisson what do you think of the PR?

To be clear, this is causing errors like this while linting, if the ESLint config uses a v7+ version of the typescript-eslint packages:

$ yarn eslint . --max-warnings 0
$ /home/runner/work/courses/courses/node_modules/.bin/eslint . --max-warnings 0
(node:2195) ExperimentalWarning: Importing JSON modules is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
Oops! Something went wrong! :(
ESLint: 9.0.0
Error: Error while loading rule 'jsx-expressions/strict-logical-expressions': You have used a rule which requires parserServices to be generated. You must therefore provide a value for the "parserOptions.project" property for @typescript-eslint/parser.
Occurred while linting /home/runner/work/courses/courses/eslint.config.js
    at Object.getParserServices (/home/runner/work/courses/courses/node_modules/@typescript-eslint/utils/dist/eslint-utils/getParserServices.js:21:15)
    at create (/home/runner/work/courses/courses/node_modules/eslint-plugin-jsx-expressions/dist/rules/strict-logical-expressions.js:60:52)
    at Object.create (/home/runner/work/courses/courses/node_modules/@typescript-eslint/utils/dist/eslint-utils/RuleCreator.js:38:20)
    at createRuleListeners (/home/runner/work/courses/courses/node_modules/eslint/lib/linter/linter.js:1003:21)
    at /home/runner/work/courses/courses/node_modules/eslint/lib/linter/linter.js:1129:84
    at Array.forEach (<anonymous>)
    at runRules (/home/runner/work/courses/courses/node_modules/eslint/lib/linter/linter.js:1061:34)
    at Linter._verifyWithFlatConfigArrayAndWithoutProcessors (/home/runner/work/courses/courses/node_modules/eslint/lib/linter/linter.js:1910:31)
    at Linter._verifyWithFlatConfigArray (/home/runner/work/courses/courses/node_modules/eslint/lib/linter/linter.js:2046:21)
    at Linter.verify (/home/runner/work/courses/courses/node_modules/eslint/lib/linter/linter.js:1535:61)

@vossmalte
Copy link

also needs eslint v9 as peer soon

@@ -24,17 +24,17 @@
"README.md"
],
"dependencies": {
"@typescript-eslint/utils": "^6.10.0",
"@typescript-eslint/utils": "^7.4.0",
"tsutils": "^3.21.0"

Choose a reason for hiding this comment

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

If you have the time, swapping this to ts-api-utils would be nice. @typescript-eslint/* packages already use ts-api-utils anyway so it'd just be removing an old, no-longer-maintained dependency.

karlhorky added a commit to upleveled/eslint-config-upleveled that referenced this pull request May 19, 2024
Alternative to waiting for the PR with support for new versions of
typescript-eslint:

- hluisson/eslint-plugin-jsx-expressions#17
@karlhorky
Copy link
Contributor

karlhorky commented May 19, 2024

For anyone who is wanting to upgrade to ESLint 9 without waiting for this PR, you could consider vendoring the published code by copying it into your project into a folder eg. vendor/eslint-plugin-jsx-expressions and deleting the package.json file (as long as you have a dependency in your project on the necessary typescript-eslint packages).

We did this in our config in this PR, and it worked well:

@boroth
Copy link

boroth commented Jul 17, 2024

Would love to see this updated for v9 as well.

@carakessler
Copy link

Ditto!

@karlhorky
Copy link
Contributor

karlhorky commented Aug 17, 2024

Workaround

Switch from eslint-plugin-jsx-expressions to @typescript-eslint/strict-boolean-expressions, with the following config:

{
  "rules": {
    "@typescript-eslint/strict-boolean-expressions": ["error", {
      "allowString": false,
      "allowNumber": false
    }]
  }
}

typescript-eslint Playground

On this code:

let num = 0;
let str = '';

<>
  <div>{num && 'x'}</div>
  <div>{str && 'x'}</div>
</>

The following will be reported:

num in num && 'x'

Unexpected number value in conditional. An explicit zero/NaN check is required. 5:9 - 5:12
> Change condition to check for 0 (`value !== 0`)
> Change condition to check for NaN (`!Number.isNaN(value)`)
> Explicitly cast value to a boolean (`Boolean(value)`)

str in str && 'x'

Unexpected string value in conditional. An explicit empty string check is required. 6:9 - 6:12
> Change condition to check string's length (`value.length !== 0`)
> Change condition to check for empty string (`value !== ""`)
> Explicitly cast value to a boolean (`Boolean(value)`)

Caveat: Also applies to other boolean expressions

This may have a downside for your project though - problems will be reported also on if conditions:

typescript-eslint Playground

Eg. on this code, 2 problems will be reported on both num and 'x' in the if condition:

let num = 0;

if (num && 'x') { }

@karlhorky
Copy link
Contributor

typescript-eslint v8 was released on 31 July 2024:

So it would probably be good to update to v8 instead of v7, in case this PR will go ahead.

cc @hluisson

@JH1ller
Copy link
Author

JH1ller commented Aug 22, 2024

@karlhorky Sorry, I didn't have the time to update this PR & we're not using this plugin anymore.

@JH1ller JH1ller closed this Aug 22, 2024
@karlhorky
Copy link
Contributor

karlhorky commented Aug 22, 2024

@JoshuaKGoldberg would typescript-eslint be open to absorbing this constrained functionality into the @typescript-eslint/strict-boolean-expressions rule as an option?

Eg. a new object?

{
  "rules": {
    "@typescript-eslint/strict-boolean-expressions": ["error", {
      "allowString": true,
      "allowNumber": true,

      "jsxExpressions": {
        "allowString": false,
        "allowNumber": false
      }
    }]
  }
}

This would aim to avoid this downside / caveat of the boolean expression rules applying to things outside of JSX?

If so, I'll open a new typescript-eslint issue.

@JoshuaKGoldberg
Copy link

I don't know, we'd have to see an issue. Not trying to be cagey but I haven't really thought this through 😅. Thanks for the offer, that'd be great.

@karlhorky
Copy link
Contributor

karlhorky commented Aug 27, 2024

@JoshuaKGoldberg Ahh there was already an issue from 2020 that actually inspired @hluisson to create eslint-plugin-jsx-expressions, I've commented over there:

@karlhorky
Copy link
Contributor

karlhorky commented Aug 27, 2024

Oh wait, looking carefully at False positive for react/jsx-no-leaked-render (jsx-eslint/eslint-plugin-react#3292), it appears that the new eslint-plugin-react-x (GitHub, docs) by @rEl1cx may have resolved these problems:

@eslint-react/no-leaked-conditional-rendering rule:

eslint.config.js

// @ts-check
 
import js from "@eslint/js";
import react from "@eslint-react/eslint-plugin";
import * as parser from "@typescript-eslint/parser";
 
export default [
  js.configs.recommended,
  {
    languageOptions: {
      parserOptions: {
        parser: tsParser,
        project: "./tsconfig.json", // <-- Point to your project's "tsconfig.json" or create a new one.
        tsconfigRootDir: import.meta.dirname,
      },
    },
  }
  {
    files: ["**/*.{ts,tsx}"],
    ...react.configs["recommended-type-checked"], // <-- Requires type information
  },
  {
    files: ["**/*.{ts,tsx}"],
    rules: {
      "@eslint-react/no-leaked-conditional-rendering": "error", // <-- Requires type information
    },
  },
];

Failing:

interface ExampleProps {
  items: string[];
}

function Example({ items }: ExampleProps) {
  return <div>{items.length && <List items={items} />}</div>;
  //           ^^^^^^^^^^^^
  //           - Potential leaked value 'items.length' that might cause unintentionally rendered values or rendering crashes.
}

declare const List: React.ComponentType<{ items: string[] }>;

Passing:

interface ExampleProps {
  items: string[];
}

function Example({ items }: ExampleProps) {
  return <div>{!!items.length && <List items={items} />}</div>;
}

declare const List: React.ComponentType<{ items: string[] }>;

Thanks @rEl1cx, I will give this a try in eslint-config-upleveled:

@karlhorky
Copy link
Contributor

I switched successfully from eslint-plugin-jsx-expressions to react-x/no-leaked-conditional-rendering, tested and confirmed to be working 🎉

If you want to see the PR and diff:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Peer dependency for @typescript-eslint/parser@^7.0.0
6 participants