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

feat: add no-required-prop-with-default rule #1943

Merged
merged 5 commits into from Sep 20, 2022
Merged

Conversation

neferqiqi
Copy link
Contributor

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.

@xiaoxiangmoe
Copy link

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

@neferqiqi neferqiqi changed the title feat: add optional-props-using-with-defaults feat: add prefer-optional-props-using-with-defaults Aug 8, 2022
sodatea
sodatea previously requested changes Aug 9, 2022
docs/rules/prefer-optional-props-using-with-defaults.md Outdated Show resolved Hide resolved
docs/rules/prefer-optional-props-using-with-defaults.md Outdated Show resolved Hide resolved
@ota-meshi
Copy link
Member

ota-meshi commented Aug 12, 2022

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.

@sodatea
Copy link
Member

sodatea commented Aug 12, 2022

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?

@ota-meshi
Copy link
Member

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
Copy link
Member

@neferqiqi can you change this PR?

@ota-meshi ota-meshi marked this pull request as draft August 23, 2022 11:05
@neferqiqi
Copy link
Contributor Author

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? :)

@ota-meshi
Copy link
Member

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

@neferqiqi
Copy link
Contributor Author

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.

@FloEdelmann FloEdelmann changed the title feat: add prefer-optional-props-using-with-defaults feat: add no-required-prop-with-default rule Sep 13, 2022
Copy link
Member

@FloEdelmann FloEdelmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, apart from some smaller remarks. Is this now ready to be merged?

docs/rules/no-required-prop-with-default.md Outdated Show resolved Hide resolved
docs/rules/no-required-prop-with-default.md Outdated Show resolved Hide resolved
lib/rules/no-required-prop-with-default.js Outdated Show resolved Hide resolved
@ota-meshi ota-meshi marked this pull request as ready for review September 15, 2022 10:00
lib/rules/no-required-prop-with-default.js Outdated Show resolved Hide resolved
lib/rules/no-required-prop-with-default.js Outdated Show resolved Hide resolved
lib/rules/no-required-prop-with-default.js Outdated Show resolved Hide resolved
Copy link
Member

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you!

@ota-meshi ota-meshi merged commit a4e807c into vuejs:master Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants