jsx-eslint / eslint-plugin-react

React-specific linting rules for ESLint
MIT License
8.88k stars 2.76k forks source link

button-has-type is not allowing a ternary statement #2969

Closed jason-chen-kiewit closed 3 years ago

jason-chen-kiewit commented 3 years ago

I'm trying to add a ternary statement to dynamically determine which type to apply to this button component. According to https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/button-has-type.md, this should work?

The error message:

The button type attribute must be a static string

My code:

import React from 'react';
import {
  string,
  bool,
  arrayOf,
  node,
  func,
  oneOf,
  oneOfType,
} from 'prop-types';
import styles from './StyledButton.module.css';

const StyledButton = ({ children, size, type, colorScheme, ...otherProps }) => {
  const assignClassName = () => {
    const style = [styles.button];
    if (colorScheme) {
      style.push(styles[colorScheme]);
    }
    if (size) {
      style.push(styles[size]);
    }
    return style.join(' ');
  };

  const test = false;
  return (
    <button
      className={assignClassName(colorScheme, size)}
      type={test ? 'submit' : 'button'} // <================ why doesn't this work?
      {...otherProps}
    >
      {children}
    </button>
  );
};

StyledButton.propTypes = {
  children: oneOfType([arrayOf(node), node]).isRequired,
  onClick: func.isRequired,
  size: string.isRequired,
  type: bool.isRequired,
  colorScheme: oneOf(['default', 'bright', 'vivid']).isRequired,
};

export default StyledButton;

image

ljharb commented 3 years ago

I believe, because ...otherProps after it might contain type. What happens if you move type to be after ...otherProps?

jason-chen-kiewit commented 3 years ago

I just tried moving it as you suggested and removing otherProps entirely. Neither worked and there was no change to the error description. =/

ljharb commented 3 years ago

What version of the plugin are you using?

jason-chen-kiewit commented 3 years ago

"eslint-plugin-react": "^7.23.2",

ljharb commented 3 years ago

Indeed https://github.com/yannickcr/eslint-plugin-react/commit/d8741de74da0fb7a0cb730f0fea14de05e4faa9b should be in v7.21.0+ via #2748.

(cc @Hypnosphi)

This seems like a bug.

Hypnosphi commented 3 years ago

@jason-chen-kiewit please share your eslint config. I can't reproduce it with just "react/button-has-type": "error"

jason-chen-kiewit commented 3 years ago

Thanks for looking into this!

eslintrc.json

{
  "env": {
    "browser": true,
    "es6": true
  },
  "extends": [
    "plugin:react/recommended",
    "airbnb",
    "plugin:prettier/recommended"
  ],
  "globals": {
    "Atomics": "readonly",
    "SharedArrayBuffer": "readonly"
  },
  "parser": "babel-eslint",
  "parserOptions": {
    "ecmaFeatures": {
      "jsx": true
    },
    "ecmaVersion": 2018,
    "sourceType": "module"
  },
  "plugins": ["react"],
  "rules": {
    "react/jsx-filename-extension": [
      1,
      {
        "extensions": [".js", ".jsx"]
      }
    ],
    "prettier/prettier": ["error", { "endOfLine": "auto" }]
  }
}
Hypnosphi commented 3 years ago

I tried to replicate your setup and I only get an error from react/jsx-props-no-spreading https://github.com/Hypnosphi/eslint-button-type-ternary

% npm run lint

> untitled@1.0.0 lint /Users/jetbrains/IdeaProjects/untitled
> eslint .

/Users/jetbrains/IdeaProjects/untitled/index.js
  30:7  error  Prop spreading is forbidden  react/jsx-props-no-spreading

✖ 1 problem (1 error, 0 warnings)
ljharb commented 3 years ago

@jason-chen-kiewit what is the version number in node_modules/eslint-plugin-react/package.json? iow, are you sure you're using the latest?

jason-chen-kiewit commented 3 years ago

@ljharb please see below.

{
  "_from": "eslint-plugin-react@latest",
  "_id": "eslint-plugin-react@7.23.2",
  "_inBundle": false,
  "_integrity": "sha512-AfjgFQB+nYszudkxRkTFu0UR1zEQig0ArVMPloKhxwlwkzaw/fBiH0QWcBBhZONlXqQC51+nfqFrkn4EzHcGBw==",
  "_location": "/eslint-plugin-react",
  "_phantomChildren": {
    "call-bind": "1.0.2",
    "define-properties": "1.1.3",
    "es-to-primitive": "1.2.1",
    "esutils": "2.0.3",
    "function-bind": "1.1.1",
    "get-intrinsic": "1.1.1",
    "has": "1.0.3",
    "is-core-module": "2.2.0",
    "is-negative-zero": "2.0.1",
    "is-string": "1.0.5",
    "object-keys": "1.1.1",
    "path-parse": "1.0.6",
    "unbox-primitive": "1.0.1"
  },
  "_requested": {
    "type": "tag",
    "registry": true,
    "raw": "eslint-plugin-react@latest",
    "name": "eslint-plugin-react",
    "escapedName": "eslint-plugin-react",
    "rawSpec": "latest",
    "saveSpec": null,
    "fetchSpec": "latest"
  },
  "_requiredBy": [
    "#DEV:/",
    "#USER"
  ],
  "_resolved": "https://registry.npmjs.org/eslint-plugin-react/-/eslint-plugin-react-7.23.2.tgz",
  "_shasum": "2d2291b0f95c03728b55869f01102290e792d494",
  "_spec": "eslint-plugin-react@latest",
  "_where": "C:\\Users\\Jason.Chen\\repos\\larry",
  "author": {
    "name": "Yannick Croissant",
    "email": "yannick.croissant+npm@gmail.com"
  },
  "bugs": {
    "url": "https://github.com/yannickcr/eslint-plugin-react/issues"
  },
  "bundleDependencies": false,
  "dependencies": {
    "array-includes": "^3.1.3",
    "array.prototype.flatmap": "^1.2.4",
    "doctrine": "^2.1.0",
    "has": "^1.0.3",
    "jsx-ast-utils": "^2.4.1 || ^3.0.0",
    "minimatch": "^3.0.4",
    "object.entries": "^1.1.3",
    "object.fromentries": "^2.0.4",
    "object.values": "^1.1.3",
    "prop-types": "^15.7.2",
    "resolve": "^2.0.0-next.3",
    "string.prototype.matchall": "^4.0.4"
  },
  "deprecated": false,
  "description": "React specific linting rules for ESLint",
  "devDependencies": {
    "@types/eslint": "^7.2.8",
    "@types/estree": "^0.0.47",
    "@types/node": "^14.14.37",
    "@typescript-eslint/parser": "^2.34.0",
    "aud": "^1.1.4",
    "babel-eslint": "^8.2.6",
    "eslint": "^3 || ^4 || ^5 || ^6 || ^7",
    "eslint-config-airbnb-base": "^14.2.1",
    "eslint-plugin-eslint-plugin": "^2.3.0",
    "eslint-plugin-import": "^2.22.1",
    "espree": "^3.5.4",
    "istanbul": "^0.4.5",
    "markdown-magic": "^1.0.0",
    "mocha": "^5.2.0",
    "semver": "^6.3.0",
    "sinon": "^7.5.0",
    "typescript": "^3.9.9",
    "typescript-eslint-parser": "^20.1.1"
  },
  "engines": {
    "node": ">=4"
  },
  "files": [
    "LICENSE",
    "README.md",
    "index.js",
    "lib"
  ],
  "greenkeeper": {
    "ignore": [
      "semver"
    ]
  },
  "homepage": "https://github.com/yannickcr/eslint-plugin-react",
  "keywords": [
    "eslint",
    "eslint-plugin",
    "eslintplugin",
    "react"
  ],
  "license": "MIT",
  "main": "index.js",
  "name": "eslint-plugin-react",
  "peerDependencies": {
    "eslint": "^3 || ^4 || ^5 || ^6 || ^7"
  },
  "repository": {
    "type": "git",
    "url": "git+https://github.com/yannickcr/eslint-plugin-react.git"
  },
  "scripts": {
    "generate-list-of-rules": "md-magic --path README.md",
    "generate-list-of-rules:check": "npm run generate-list-of-rules && git diff --exit-code README.md",
    "lint": "eslint ./",
    "postlint": "npm run type-check",
    "posttest": "aud --production",
    "pretest": "npm run lint",
    "test": "npm run unit-test",
    "type-check": "tsc",
    "unit-test": "istanbul cover node_modules/mocha/bin/_mocha tests/lib/**/*.js tests/util/**/*.js tests/index.js"
  },
  "version": "7.23.2"
}
jason-chen-kiewit commented 3 years ago

@Hypnosphi thanks for trying to replicate it, I think this means that there is something specific in my setup that is not right...

Would love to hear ideas on where to start looking though. ><;

ljharb commented 3 years ago

Wait - you’re getting this error in your ide, not on the command line?

ljharb commented 3 years ago

The command line is the source of truth; if it works properly there it’s an issue with the editor. Note that every editor I’m aware of needs to be restarted after an npm install before it picks up the new eslint stuff.

jason-chen-kiewit commented 3 years ago

@ljharb You're right! I have husky to prevent the commit from happening if it finds an error and it looks like it went through no problem.

image

still weird... but I'll take it.

ljharb commented 3 years ago

Does restarting your editor fix the problem?

jason-chen-kiewit commented 3 years ago

@ljharb Yes it does... I had updated to the newest version and I didn't think to restart my vscode. 🤦

Thank you for all the help regardless!