eslint-plugin-vue icon indicating copy to clipboard operation
eslint-plugin-vue copied to clipboard

feat: add prefer-optional-props-using-with-defaults

Open neferqiqi opened this issue 3 years ago • 1 comments

This rule enforce props with default values ​​to be optional. Because when a required prop declared with a default value, but it doesn't be passed value when using it, it will be assigned the default value. So a required prop with default value is same as a optional prop.

neferqiqi avatar Aug 08 '22 12:08 neferqiqi

Maybe we should add this rule in vue2 and vue3 presets.

xiaoxiangmoe avatar Aug 08 '22 12:08 xiaoxiangmoe

EDIT: ~~I misunderstood. Forget this comment.~~ EDIT: Sorry. My previous understanding is correct. I think it would be useful if the rule could check for other cases as well.


Thank you for this PR.

I think a rule that checks for unnecessary defaults would be useful. However, I think it would be better to be able to check other than <script setup lang="ts">.

<script>
export default {
  props: {
    foo: {
      required: true,
      default: 42
    }
  }
}
</script>
<script setup>
defineProps({
  foo: {
    required: true,
    default: 42
  }
})
</script>

In this case, don't use withDefaults, so maybe we should rename the rule.

ota-meshi avatar Aug 12 '22 00:08 ota-meshi

Regarding the design: eslint-plugin-react uses an option in require-default-props for a similar purpose: https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/require-default-props.md#forbiddefaultforrequired

I think that design might be more performant as all the checks are done in one pass. What do you think?

haoqunjiang avatar Aug 12 '22 03:08 haoqunjiang

Hmm... I think it's a good idea to follow the precedent, but I think the name require-default-props can be confusing to report unnecessary defaults. If I had to choose one, I think the new rule is better.

ota-meshi avatar Aug 12 '22 04:08 ota-meshi

@neferqiqi can you change this PR?

ota-meshi avatar Aug 23 '22 11:08 ota-meshi

ok, i will change the rule to supoort the case

<script>
export default {
  props: {
    foo: {
      required: true,
      default: 42
    }
  }
}
</script>

but i want to reserve the auto-fix. Because firstly, it only fix the props in current file, so it's safe to auto fix; secondly, for example in my project, there are many cases that assign default values ​​to required props, i think need a auto fix way to fix these cases. what do you think? :)

neferqiqi avatar Aug 24 '22 03:08 neferqiqi

but i want to reserve the auto-fix. Because firstly, it only fix the props in current file, so it's safe to auto fix; secondly, for example in my project, there are many cases that assign default values ​​to required props, i think need a auto fix way to fix these cases. what do you think? :)

I don't think it should auto-fix. As I wrote earlier, it's because we don't know which of the two methods the user intended. I think it's the same reason no-unused-vars doesn't automatically remove variables. It's safe to remove the variable, but we shouldn't, as the user may intend to use it.

However, I do understand that your team need auto-fix. What do you think of adding an option to enable auto-fix? (However, that in most cases I don't like that option as it just complicates the implementation of the rules.)

ota-meshi avatar Aug 24 '22 04:08 ota-meshi

but i want to reserve the auto-fix. Because firstly, it only fix the props in current file, so it's safe to auto fix; secondly, for example in my project, there are many cases that assign default values ​​to required props, i think need a auto fix way to fix these cases. what do you think? :)

I don't think it should auto-fix. As I wrote earlier, it's because we don't know which of the two methods the user intended. I think it's the same reason no-unused-vars doesn't automatically remove variables. It's safe to remove the variable, but we shouldn't, as the user may intend to use it.

However, I do understand that your team need auto-fix. What do you think of adding an option to enable auto-fix? (However, that in most cases I don't like that option as it just complicates the implementation of the rules.)

ok, i will add an option to enable auto-fix.

neferqiqi avatar Aug 24 '22 06:08 neferqiqi