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

Recognize tslint-react as an extended ruleset #656

Merged
merged 2 commits into from
Aug 9, 2020

Conversation

yasarsid
Copy link
Contributor

@yasarsid yasarsid commented Jul 29, 2020

PR Checklist

Overview

Recognize tslint-react as a extended ruleset.

Github Issue

If a rule extends from tslint-react , we will now add react/recommended to the configuration.

Recommendation from eslint-plugin-react are present Here. I have left out eslint:recommended not sure if that would be required.

Have added unit tests and verified if the coverage remains at 100%.

@KingDarBoja KingDarBoja added the status: waiting for reviewer Waiting for a maintainer to review label Aug 4, 2020
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Thanks for sending this PR @yasarsid! The changes so far look good.

However, when trying this out locally, I'm seeing this come up as an error in tslint-to-eslint-config.log:

Could not resolve ESLint extension 'plugin:react/recommended'.: Error: Cannot find module 'C:\Code\tslinttest\plugin:react\recommended'

You'll need to add an entry to src/creation/summarization/retrieveExtendsValues.ts that tells tslint-to-eslint-config how to look up extended rules from the ESLint react ruleset.

@JoshuaKGoldberg JoshuaKGoldberg added status: waiting for author The PR author should address requested changes and removed status: waiting for reviewer Waiting for a maintainer to review labels Aug 8, 2020
@yasarsid
Copy link
Contributor Author

yasarsid commented Aug 9, 2020

@JoshuaKGoldberg Thanks for an awesome review and pointing out the problem ( along with the fix for it 👍 )

I made the changes and tested them locally - I am sharing a few findings


Additional error message

⛔ I did see an error in tslint-to-eslint-config.log but I think that is unrelated to my changes. ⛔

I wanted to be sure hence bring this up here -

Error

Error: multiple output @typescript-eslint/naming-convention ESLint rule options were generated, but tslint-to-eslint-config doesn't have "merger" logic to deal with this.
Please file an issue at https://github.com/typescript-eslint/tslint-to-eslint-config/issues/new?template=missing_merger.md. Thanks!

Rulesets for react/recommended

The eslintrc.js generate had the following entries and these match the recommended rules for ESLint-plugin-React
Verified in node_modules and the github link

Entries of recommended rules -

        "react/display-name": "error",
        "react/jsx-boolean-value": "error",
        "react/jsx-key": "error",
        "react/jsx-no-comment-textnodes": "error",
        "react/jsx-no-duplicate-props": "error",
        "react/jsx-no-target-blank": "error",
        "react/jsx-no-undef": "error",
        "react/jsx-uses-react": "error",
        "react/jsx-uses-vars": "error",
        "react/no-children-prop": "error",
        "react/no-danger-with-children": "error",
        "react/no-deprecated": "error",
        "react/no-direct-mutation-state": "error",
        "react/no-find-dom-node": "error",
        "react/no-is-mounted": "error",
        "react/no-render-return-value": "error",
        "react/no-string-refs": "error",
        "react/no-unescaped-entities": "error",
        "react/no-unknown-property": "error",
        "react/no-unsafe": "off",
        "react/prop-types": "error",
        "react/react-in-jsx-scope": "error",
        "react/require-render-return": "error",

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Perfect, thanks!

@JoshuaKGoldberg
Copy link
Member

So thorough, this is great!

Yes, #666 is filed separately for the missing naming-convention merger. Much appreciated!

@JoshuaKGoldberg JoshuaKGoldberg merged commit a55bbfb into typescript-eslint:master Aug 9, 2020
@JoshuaKGoldberg JoshuaKGoldberg removed the status: waiting for author The PR author should address requested changes label Aug 9, 2020
@JoshuaKGoldberg JoshuaKGoldberg added this to the Next milestone Aug 9, 2020
@yasarsid yasarsid deleted the tslint-react branch August 9, 2020 15:51
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.

Recognize tslint-react as an extended ruleset
3 participants