eslint-plugin-react: button-has-type does not allow for dynamic assignment

The type should not be dynamic, full stop.

If you want the type to be one of the 1, 2, or 3 options, use if/else and hardcode those three.

Why shouldn’t it be dynamic? Can you explain please?

Stateless React Components should not use JS default values; that needs to be a defaultProp.

@musicMan1337

export const Button = ({ className, type, ...props }) => {
  if (type === 'button') {
    return (
      <button
        className={['button', className].join(' ')}
        type="button"
        {...props}
      />
    );
  }
  if (type === 'submit') {
    return (
      <button
        className={['button', className].join(' ')}
        type="submit"
        {...props}
      />
    );
  }
};
Button.propTypes = {
  className: PropTypes.string,
  type: PropTypes.oneOf(['button', 'submit']),
};
Button.defaultProps = { className: '', type: 'button' };

There is never a need to dynamically specify a Button’s type (and you never ever want a “reset” button, so those two are sufficient).

I think we can agree to disagree on that so I’ll close this issue here. A workaround was mentioned that works just fine. Thanks for all the work on this plugin!

@lydell very true! the ternary case would probably have to be explicitly handled; however, I’d say that the rule should be forcing a single, hardcoded type value - and that you should conditionally render two different elements if you want two types.

Just to add, the above “solution” doesn’t work for TypeScript.

interface Props {
  type: 'button' | 'submit' | 'reset';
}

const Button: React.StatelessComponent<Props> = ({
  children,
  type = 'button',
}) => (
  <button type={type}>{children}</button> 
);

I’m going to just assume I’ll have to disable the rule.

Only twice, since reset buttons are a terrible UX pattern 😃

It shouldn’t be dynamic because all three button types have wildly different semantics - i can’t conceive of when you’d want a submit button randomly changing to be an entirely different kind of button, or vice versa.

A submit button must be inside a form; a regular button must have some kind of javascript behavior - these are different components/elements, not just two flavors of the same thing.

It shouldn’t be dynamic because all three button types have wildly different semantics - i can’t conceive of when you’d want a submit button randomly changing to be an entirely different kind of button, or vice versa.

Commonly people create a Button element of which it’s main purpose is to style a button and handle all the different states of the button. This button could then be used in a form with the type ‘submit’. Another use-case would be a button with an onClick handler. This button would need the type ‘button’. I’m pretty sure these two use-cases are spec compliant.

You see, that’s exactly the answer I was afraid would appear any time in this thread.

Many users get confused by the rule. Judging by all your arguments (against “reset” types) the rule should maybe have been named button-has-correct-type and correct means one and foremost “not reset”. All the use cases users presented are basically dismissed by you by asking the users to change their code (into needlessly convoluted if/else structs) just to get rid of the false positive.

Responding to valid concerns regarding the usage of a rule should not be countered by “you can fork” or lecturing users on their code.

I am beyond this, as I do currently not work with react anymore but this “you can fork” just sounded rude to me considering all the above constructive feedback on this very opinionated rule.

But then maybe its also not easily possible to change the rule to support dynamic types (“reset” not included) even if the maintainers wanted to.

I agree this rule is handled poorly. defaultProps with a type of either button, reset, or submit should not trigger.

A work-around is to just disable the rule temporarily:

/* eslint-disable react/button-has-type */
return <button {...passProps} className={buttonClassNames} type={type}/>;
/* eslint-enable react/button-has-type */

How would using this option be different than simply disabling the rule, or using an override comment? The rule, despite it’s name, does not merely care if your button has a type attribute, the point is to assert it’s a correct one - which your option does not ensure.

defaultProps will be deprecated on function components.

I have this use case:

function Button({children, type = 'button'}) {
  return <button type={type}>{children}</button>;
}

At least an opt-in for dynamic assignment would be great. The only option is for me to disable the rule.

@Bram-Zijp sure. and the implementation of that Button element can still use hardcoded types with an if/else, or, use an eslint override comment.

@amannn the other option is to use an if/else with a hardcoded type, which would help ensure you’re using the correct type, and not using reset.

type ButtonProps = React.ButtonHTMLAttributes<HTMLButtonElement> & {
  /**
   * enforcing the type attribute to be required and limited to "submit" and "button"
   */
  type: 'submit' | 'button';
};

export const Button = (props: ButtonProps) => {
  // see ButtonProps
  // eslint-disable-next-line react/button-has-type
  return <button {...props} />;
};

const Btn = () => <Button />; // <- ts error

my 2cents on this.

but then you’ll be writing your runtime checks in addition to compile/test-time checks, if you’re that concerned with correctness.

Every decent software engineer should be that concerned with correctness. And no static type analysis or ASSERT will prevent invalid input. But I’m afraid this kind attitude is one the reasons there is so many sloppy insecure software released today.

On the other hand you did not answer my question if the team would accept a PR with the option to enabled dynamic type assigned for the many reasons voiced and voted here by users of the plugin? The option would default to the current restrictive behaviour but could be relaxed.

I don’t agree; an eslint override comment is the appropriate non-hacky solution to a false positive eslint warning.

I don’t remember @vaske 😕 Maybe I just disabled the rule.

This thread is hilarious. Thanks for all your input @ljharb and contributions to open source, but you are enforcing your opinions on everyone else and saying ‘deal with it’. Eslint is designed to be non-opinionated (unless you are using opinionated plugins/presets) and customisable to the users’ needs. Here you are saying ‘I don’t agree with everyone else so I’m not going to give them the option to do what they need to do, tough luck’.

@pke my response saying to fork was to “eslint is supposed to be unopinionated”, any package can be as opinionated as it wants to - this plugin is opinionated like every other.

I totally agree that changing the rule name would help, but the churn of doing that isn’t worth it.

It’s not at all practical and possible to support dynamic types; that’s just the nature of JavaScript itself (it could be supported in extremely limited ways that would require massive complexity, but that wouldn’t be sufficient for virtually any of the use cases folks have complained about)

If your propType warnings are properly set up to fail your tests, there’s no need to check it in production.

@musicMan1337 you can only defaultProps outside the component. Your statement doesn’t take effect until after the component has been rendered.

@josephshambrook at the least, make a propType that used oneOf, so you’re not just wildly accepting any random string (or worse, “reset”).

Using an override comment inside a component where you’re knowingly violating a best practice is indeed the proper way to handle it.

Yep, I disabled the rule too.

const Button = ({ type }) => {

  /*
   * JSX line disabled for ESLint due to questionable rule implementation
   */

  return (
    // eslint-disable-next-line react/button-has-type
    <button type={type} className="button">Test</button>
  );
};

Button.propTypes = {
  type: PropTypes.string,
}

Button.defaultProps = {
  type: 'button',
}

In a REST level 3 kind of app where the app state is completely controlled by the server, the server dictates the fields of a form. That’s a valid scenario for when the button type needs to be dynamic. Since hardcoding the now known button types defeats the entire reason for going REST level 3, which is to have an evolutionary API, which can change in the future without touching the client but just send a new HTML6 value for button type.

But since REST level 3 apps are such a rare breed as of now I’ll just disable this rule that’s supposed to make the button type usage “safe”.

I am also interested in “why” it should be dynamic (because dev typo in props?). Seems weird to duplicate the button three times for a hardcoded value.

htmlType: PropTypes.oneOf(['button', 'submit', 'reset']),
static defaultProps = { htmlType: 'button' }
<button type={htmlType || 'button'} />

@Hypnosphi sure, seems harmless enough, as long as both branches in the ternary were static strings.

@ljharb It can, it will look hacky and it shouldn’t have to considering the legitimacy of use-cases. Perhaps an adding an option to the eslint config which allow for dynamic types would please everyone.

Yep, that’s true. To me that’s more the job of propTypes or a type system though.

Isn’t that true for input types as a whole? Not just specific button? Why wouldn’t this give an error as well?

type: PropTypes.oneOf(['text','email','tel','password','url','search','number','date','range','file']),
static defaultProps { type: 'text' }
<input type={type || 'text'} ... />