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

Boolean props should be false by default always. (require-default-prop) #222

Closed
jpickwell opened this issue Oct 24, 2017 · 12 comments
Closed

Comments

@jpickwell
Copy link

Tell us about your environment

  • ESLint Version: 4.9.0
  • eslint-plugin-vue Version: 3.13.1
  • Node Version: 8.2.1

Please show your full configuration:

What did you do? Please include the actual source code causing the issue.

<script>
export default {
  props: {
    showText: {
      type: Boolean,
      default: true
    },
    autoPlay: Boolean
  }
}
</script>

What did you expect to happen?
An error should be shown for the showText prop indicating that it should not have a default of true. The autoPlay prop should considered correct even with the require-default-prop rule turned on.

What actually happened? Please include the actual, raw output from ESLint.
With the require-default-prop rule enabled, autoPlay will be flagged. showText will be considered correct.

To make Vue components work like idiomatic HTML elements Boolean attributes/props should always default to false. If a Boolean prop defaults to true then a user who wants to turn that prop off has to bind the prop to a constant false (e.g., :show-text="false").

The require-default-prop rule would need to be modified to either enforce this, or ignore Boolean props. If the rule ignores Boolean props, then a new rule could be added to enforce proper Boolean props.

Boolean props should also never be required.

someBooleanProp: Boolean is how all Boolean props should be declared. There's no need for any other prop options.

At the very least, something should be added to the rules to allow enforcing this. I also believe this rule should be added to the Style Guide.

@michalsnik
Copy link
Member

This is a really interesting proposal @jpickwell. I agree that setting default value for Boolean property seems daunting and it makes perfect sense to ignore this type in require-default-prop rule. But validating default property's content for this type of prop sounds like a job for a new rule no-truthy-boolean-prop or something like that.

What do you think about this @chrisvfritz ? It might be a good practice to add in the styleguide, as it would definitely improve consistency in components APIs.

@chrisvfritz
Copy link
Contributor

If we do include it, I think it should be a new rule, but probably not part of the style guide. I agree that most defaults should be false, but there are legitimate exceptions. For example, you may have an alert component that you want to be dismissible 99% of the time in your app. But sometimes, you want notifications that aren't dismissible, so you make the behavior optional with a new dissmissible prop that defaults to true. Does that make sense?

@byCedric
Copy link

byCedric commented Nov 22, 2017

I really like this proposal, and to be honest I have never thought of the boolean properties this way. But it does make a lot of sense!

Let's say we have a star-rating component which simply displays the 5-star rating and the total amount of ratings. Now sometimes you only need to show the stars, without the total count. With this idiomatic stuff in mind you can either create a show-count or a hide-count depending on if you want to show the count by default or not. I think you can apply this show/hide principle to (almost) everything. The dismissible example could look like can-dismiss and cannot-dismiss for example. Even introduce with-/without- or dismissible/non-dismissible if you prefer it. Combinations are as limited as you make them yourself!

But I also understand that this is maybe too early or too "opinionated" to include this into the style guide. But as said, I really like this proposal and would love to see it as one of the available rules!

@mysticatea
Copy link
Member

I feel that this is a good idea 👍, but it should be separated from require-default-prop rule.

@niklashigi
Copy link
Contributor

But sometimes, you want notifications that aren't dismissible,
so you make the behavior optional with a new dismissible prop that defaults to true.

@chrisvfritz This prop could be named persistent making it false by default.

<alert dismissible="false">You have to accept our new terms to continue using XY.</alert>
<!-- vs -->
<alert persistent>You have to accept our new terms to continue using XY.</alert>

I can't think of a single situation where this problem cannot be solved by renaming the prop. I think that with the right error message this rule could really benefit developers, especially if it includes a tip like this:

Try to inverse the name of the prop: If the component is not '{{ currentName }}', it's ...?

If I find the time, I might have a shot at implementing this rule.

@michalsnik
Copy link
Member

Feel free to create such rule @shroudedcode ! It would be appreciated :) And we'll ignore default key in require-default-prop for Boolean props.

@chrisvfritz
Copy link
Contributor

Just checking in on all the new rule proposals. What does everything think about calling this boolean-props-default-false? I'm thinking it would stay in uncategorized though, since the benefits of consistency here seem a little more subjective.

@Worthaboutapig
Copy link

To make Vue components work like idiomatic HTML elements Boolean attributes/props should always default to false. If a Boolean prop defaults to true then a user who wants to turn that prop off has to bind the prop to a constant false (e.g., :show-text="false").

I strongly disagree with this, but unfortunately it's a common pattern of 'false by default', which was taken up by HTML with things like disabled. I don't see that this should be replicated in Vue, as Vue isn't HTML, it's meant to make designing interfaces easier and IMO, this 'negative by default' makes things much harder.

There are two problems with this 'negative by default', both stemming from not designing an interface 'positively'. Property names should represent the 'positive' state of the concept being represented (even when the most likely value is the 'false' of that state). Making a property false by default induces unnecessary extra cognitive load by making the user have to work out what the true value of a negative concept is- it's not how people think.

For example, one often sees something like this:
disabled() { return !this.isWeekday(); }

One can't look at that and immediately know the state. For instance,'Today is Tuesday? Can I click on the control?'

Whereas:
enabled() { return this.isWeekday(); }
requires no real thought to say 'Yes, it's enabled and I can use it'.

It is much better (IMHO) to work out the positive concept. After all, with something like disabled, even though it's named 'negatively', at the end of the day interfaces are designed to do things, not to not do them, so when considering a button, one doesn't care whether it's disabled, you care 'can I click on it and initiate some action'- the purpose of the button is a positive action- to do something, so it should be enabled by default and there should not be a disabled state, but an enabled state that is false.
Furthermore, this can bleed into other parts of the code where working out what's going on can get really hard when you have lots of variables each trying to decide whether a negative thing is the result you want.

Interfaces, on the whole allow you to do things, designing an interface around what you can't do, it's much of a user journey. By taking a 'positive' approach, it does force one to think what the purpose and journey of a concept is- by thinking 'how can I name this positively', it makes one think a bit more deeply about what you're trying to achieve and whether it's the appropriate way.

@Worthaboutapig
Copy link

This prop could be named persistent making it false by default.

@chrisvfritz I'm a bit confused here, am I right in thinking that on the one hand you're saying that the default value of persistent is false, thereby saying, 'by default this is not persistent and hence can be dismissed', whilst you're then saying 'actually when I say false, I mean interpret that as "true" and it can't be dismissed.'

I can't think of a single situation where this problem cannot be solved by renaming the prop.

I fully agree with this, but I can't see the sense in saying 'the default value of false means it will actually do the opposite of the natural-language meaning'.

<script>
export default {
  props: {
    showText: {
      type: Boolean,
      default: true
    },
    autoPlay: Boolean
  }
}
</script>

So, the correct implementation is:

<script>
export default {
  props: {
    hideText: Boolean,
    autoPlay: Boolean
  }
}
</script>

Again, I really don't see this. By default I'm not hiding the text. Thinking negatively about things is really hard. The default position should be to show the text and then there should be a few occasions when you don't show the text. This false approach means 'most of the time I'm not hiding the text', which again is hard to process- you have a negative value of a negative concept.

You end up with a Alice-in-Wonderland sort of situation, it's like having this conversation with someone:

Hi, what days do you go to work?
Oh, same as most people, I don't go to work when it's not a weekday.
What?? Hang on... oh, you mean you don't work at weekends- so you work weekdays? Why didn't you just say so?

You may have guessed that this is my No 1 bugbear in software dev. Hopefully I haven't misunderstood everything.

@chrisvfritz
Copy link
Contributor

chrisvfritz commented Sep 24, 2018

@Worthaboutapig It's perfectly fine not to like this style - as an uncategorized rule, the only way for it to be active would be if you explicitly chose to include it. 🙂

@ghost
Copy link

ghost commented Feb 16, 2019

It looks like this issue had been covered by the new rule no-boolean-default with option of allowing either no default for booleans at all, or only defaults that are false.

@ota-meshi
Copy link
Member

It is covered by no-boolean-default rule as @st-sloth say. So I close this issue.

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

No branches or pull requests

8 participants