vuejs / eslint-plugin-vue

Official ESLint plugin for Vue.js
https://eslint.vuejs.org/
MIT License
4.47k stars 666 forks source link

feat(v-on-handler-style): allow `["inline", "inline-function"]` option #2471

Open ByScripts opened 5 months ago

ByScripts commented 5 months ago

Fixes #2460

Allow using ["inline", "inline-function"] option for v-on-handler-style rule.

In this case:

FloEdelmann commented 5 months ago

Cool, thank you! Should the inline-function style really already be accepted with one argument (where inline would also work with $event), or only with two or more?

ByScripts commented 5 months ago

This would prevent the interdiction to use $event in favor of explicit naming:

<template>
  <!-- Using an inline function with a good argument name is better -->
  <MyComp @increase="(count) => myHandler(count)" />`

  <!-- It's why we don't allow $event, as it is meaningless (and is absolutely not an Event here) -->
  <MyComp @increase="myHandler($event)" />`
</template>

Edit: Or maybe it could be configurable as an option?

FloEdelmann commented 5 months ago

I think if you want to force explicit parameter naming, then you should only allow inline-function. It's 5 extra characters for functions without parameters (() =>), but it's a simple and effective rule.

If you prefer shortness over explicitness however, then ["inline", "inline-function"] is fine, but IMO it should only fall back to inline-function when inline does not work at all, i.e. only for functions with more than one parameter.

ByScripts commented 5 months ago

I see your point, but it's really common to only need an inline handler.

I don't think we should force users to use @event="() => handler()" where @event="handler()" is enough.

The primary goal of this PR is to forbid the usage of method handler, as it's error prone, and allow the two others.

I updated my proposal to add an allowInlineFuncSingleArg?: boolean option that can only be used in conjunction with ['method', 'inline-function'] or ['inline', 'inline-function'].

FloEdelmann commented 5 months ago

Hmm, I don't like the extra option. The logic is quite convoluted already anyway, and the extra option makes it even harder to understand.

But I think we can change the rule like this:

  1. For cases like @click="(arg1, arg2) => handler(arg1, arg2)", always allow the inline-function style, because there is no other way to represent those. Multiple event payloads should rather be reported at the emitting side, see https://github.com/vuejs/eslint-plugin-vue/issues/2005
  2. Add a new option ["inline", "inline-function"] that allows inline but disallows using $event, and allows using inline-function in that case.

What do you think?

ByScripts commented 5 months ago

Thank you for your reply.

After some thought, I wonder if this rule isn't trying to handle too many things at once.

I agree on your first point (although I think vue/event-prefer-single-payload would be a better name. At first I read "Prefer Single-Event Payload" ^^)

Perhaps it would be easier to disallow these different behaviors with new, independent rules?

For example:

  1. @click="count++" ➡️ Disallow with vue/event-no-inline-expr
  2. @click="increase" ➡️ Disallow with vue/event-no-method-handler
  3. @click="increase()" ➡️ Disallow with vue/event-no-inline-handler
  4. @click="() => increase()" ➡️ Disallow with vue/event-no-arg-less-inline-function
  5. @click="increase($event)" ➡️ Disallow with vue/no-dollar-event
  6. @click="(count) => increase(count)" ➡️ No reason to disallow?
  7. @click="(count, multiply) => increase(count, multiply)" ➡️ No reason to disallow?

This is just a quick overview, and I may be overlooking certain situations.

ota-meshi commented 4 months ago

Thanks for working on new option and for the suggestions.

However, for now I don't think it's a good idea to split up what the v-on-handler-style rule is doing, because I think it would be hard to design the options in such a way that those rules don't conflict.

FloEdelmann commented 4 months ago

@ByScripts are you interested in changing the rule like I described in my comment above?