button-has-type does not allow for dynamic assignment

[pke]

My Button component allows to override the type arg but button-has-type is still complaining about. It seems it does not validate the real AST path of prop assignment

Button = ({ type = "button" }) => <button type={type}/>
Button.propTypes = {
  type: PropTypes.arrayOf(["submit", "button", "reset"]),

Does the plugin have access to the propTypes and can match that the only possible allowed values are within the rules allowed values and that the default type is also set to something specific, like in my example to button?

[ljharb]

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

[pke]

When you say "should not" where does this recommendation come from? Is this a lint suggestion or are there language/runtime implications?

[ljharb]

defaultProps are introspectable by React and other tools; default arguments are not. #1184 has more discussion on it.

[lydell]

Regardless of the defaultProps discussion, something’s buggy with this rule.

The following code:

<button type={submit ? "submit" : "button"} onClick={onClick}>
  {children}
</button>

… gives the following lint error:

"null" is an invalid value for button type attribute

Surely it shouldn’t say "null" there?

[ljharb]

@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.

[mikebarnhardt]

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 */

[ljharb]

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.

[pke]

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

[brandonburkett]

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'} />

[ljharb]

Only twice, since reset buttons are a terrible UX pattern :-)

[ljharb]

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.

[brandonburkett]

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'} ... />

[ljharb]

Yes, that is certainly true! However, the primary point of the rule is to ensure that button is used safely.

If we wanted to make a generic rule that governed input types, that would also force them to be static (and I'd encourage you not to make a non-private generic input component that takes "type" as a parameter, for the reasons I mentioned above)

[pke]

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".

[ljharb]

I'm not familiar with any HTML 6 nor any plans for it in the near or far future, nor any plans for any new button type ever. When one appears, the server shouldn't be controlling that - it should only be explicitly supported in the UI.

[pke]

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!

[vaske]

@pke which workaround works to you?

[pke]

I don't remember @vaske :/ Maybe I just disabled the rule.

[vaske]

@pke yeah I did the same ;)

[josephshambrook]

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',
}

[ljharb]

@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.

[pke]

Since propTypes are not used in production builds anyway such safeguards against "reset" type should happen in code, not in propTypes.

[ljharb]

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

[mattdell]

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.

[vaske]

@mattdell I did the same ;)

[Stephen2]

A little gross IMO, but I'm doing the "disable the rule" thing.

[amannn]

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.

[ljharb]

@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.

[amannn]

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

[Bram-Zijp]

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.

[ljharb]

@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.

[Bram-Zijp]

@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.

[ljharb]

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

[pke]

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

OT: Excuse me, but that assumption is nonsense. Any input not in the control of the code should be checked for validity. Relaying on a dev mode only feature for stabilising an API seems risky to me.

And with the deprecation of defaultPropTypes for stateless components your recommendation for "fixing" this ill rule discussed here, outdated very quickly. And whoever followed this advise will have to fix some tests soon I guess?

But there you go. This rule long could have had a config option to allow the behaviour some users would like this rule to have. Would you accept a PR for a rule option?

[ljharb]

The assumption isn’t nonsense, it’s the justification for PropTypes not running in production, and for type systems not compiling to runtime checks. If you don’t agree, that’s also fine, but then you’ll be writing your runtime checks in addition to compile/test-time checks, if you’re that concerned with correctness.

Deprecating defaultProps for SFCs in favor of JS function defaults doesn’t have any impact on this discussion.

[pke]

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.

[ljharb]

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.

[robguy21]

The rule, despite it's name, does not merely care of your button has a type attribute,

Perhaps the name should change then to be more indicative of what it's actually doing.

A codebase I'm working on now has many different groups of buttons, and nearly all buttons are generated from an array of "object"s and then props are put onto the button as we iterate over the object.

There are definitely valid use cases to only want to check the existence of non-empty type property on a button.

If you don't want to support this that's alright but

The rule, despite it's name,

Clearly indicates a problem and should be looked at it. It's clearly misleading.

[thejamespower]

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'.

[ljharb]

You are more than welcome to make your own plugin that does whatever you want; this plugin (like every other) is allowed to be as opinionated as its maintainers want to be.

[pke]

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.

[ljharb]

@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)

[Hypnosphi]

the ternary case would probably have to be explicitly handled

This sounds like a good solution to me. Would a PR adding this be welcome?

[ljharb]

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

[musicMan1337]

I'm having the same Eslint issue, hence I found this page. Much of the conversation essentially claimed that buttons should be hard-coded due to the different 'schemas' needed for different button types. I came up with this:

export const Button = ({ className, type, ...props }) => {
  Button.defaultProps = { className: '', type: 'button' };

  /* eslint-disable react/button-has-type */
  return (
    <button
      className={['button', className].join(' ')}
      type={type}
      {...props}
    />
  );
};

As you can see, I was forced to disable the linter here, but as far as dynamic, this is about as good as I could make it to support every style of button via overloading, including if you wanted to add additional class names.

I've only been coding for about 2 months, so any feedback (positive AND constructive) would be greatly appreciated!

[Hypnosphi]

@musicMan1337 do you ever use <Button type="submit" />? If not, you can just remove the type prop from Button and specify <button type="button" /> explicitly:

export const Button = ({ className, ...props }) => {
  Button.defaultProps = { className: '' };

  return (
    <button
      className={['button', className].join(' ')}
      type="button"
      {...props}
    />
  );
};

[ljharb]

@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).

[musicMan1337]

To preface, I'm not the best with forms!

@Hypnosphi I've found in React I tend to only use submit buttons when I actually need a real button. If I'm just testing functionality with dummy buttons then it doesn't matter, then again I'm sure you can do wonderous effects using buttons!

@ljharb It's interesting how you feel about the "reset" type, I'll have to look into that some more since I have used them in other small projects, so thanks for the nod! And as for the "if" statement, I definitely see the logic, but I personally find it very cumbersome to repeat code in that way, particularly from a readability perspective.

Also, I didn't know you could put default props outside of the component, I'll have to play with that, thanks!

[ljharb]

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

[musicMan1337]

@ljharb Oh nice that's good to know! I was being misdirected by my linter since the error turns off both ways (I guess in that specific case it just scrubs the whole file without scope?).

Definitely need more practice with React, most of my time has been spent on backend stuff. Also I now clearly understand the taboo of the "reset" button type!