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(v-on-handler-style): allow ["inline", "inline-function"] option #2471

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ByScripts
Copy link

Fixes #2460

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

In this case:

  • method style will be forbidden
    • @click="myHandler"
  • inline-function style will only be accepted with at least 1 argument:
    • @click="() => myHandler()"
    • @click="myHandler()"
    • @click="(evt) => myHandler(evt)"
    • @click="(arg1, arg2) => myHandler(arg1, arg2)"
  • inline style will be required in other cases
    • @click="() => myHandler($event)"
    • @click="myHandler($event)"
    • @click="() => count++"
    • @click="count++"

@FloEdelmann
Copy link
Member

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

ByScripts commented Jun 6, 2024

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

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

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

  • When allowInlineFuncSingleArg is true:

    () => handleFilter()
    => handleFilter() if ['inline', 'inline-function']
    => handleFilter if ['method', 'inline-function']

    (filter) => handleFilter(filter)

    (filter, negate) => handleFilter(filter, negate)

  • When allowInlineFuncSingleArg is false (by default)

    () => handleFilter()
    => handleFilter() if ['inline', 'inline-function']
    => handleFilter if ['method', 'inline-function']

    (filter) => handleFilter(filter) => handleFilter($event)
    => handleFilter($event) if ['inline', 'inline-function']
    => handleFilter if ['method', 'inline-function']

    (filter, negate) => handleFilter(filter, negate)

@ByScripts ByScripts force-pushed the feature/#2460-allow-inline-inline-function-v-on-handler-style branch from e95c657 to 91da522 Compare June 8, 2024 18:41
@FloEdelmann
Copy link
Member

FloEdelmann commented Jun 19, 2024

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 Rule suggestion: vue/prefer-single-payload-in-event #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
Copy link
Author

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

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

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

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.

Suggestion: allow ["inline", "inline-function"] option for v-on-handler-style
3 participants