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

edit v-on-function-call to enforce @click="() => fn()" over @click="fn()" and @click="fn" #2001

Closed
mesqueeb opened this issue Oct 5, 2022 · 8 comments · Fixed by #2009
Closed

Comments

@mesqueeb
Copy link
Contributor

mesqueeb commented Oct 5, 2022

https://eslint.vuejs.org/rules/v-on-function-call.html#vue-v-on-function-call

I want to have a new option for v-on-function-call to enforce a 3rd way of writing that is not yet enforcable. My reasoning is very sound I believe bet let me elaborate:

Please describe what the rule should do:

There are 3 ways in Vue to pass an event listener in HTML:

<button @click="fn" />
<button @click="fn()" />
<button @click="() => fn()" />    

To me there are clear PROs for using the last way and only CONs for using the first two. This is why I want to enforce this style with a new rule.

Here is my reasoning:

<button @click="fn" />
  • CON: it's unclear to see what payload comes out of the event and what payload goes into the fn if any.
  • CON: sometimes fn doesn't accept a payload, but will still receive one because of this way of writing
  • CON: sometimes fn is used somewhere else with a payload, but passed here as well, but it will end up receiving PointerEvent | MouseEvent implicitly which can cause bugs that are hard to find because it's implicit
  • CON: a lot of VSCode themes color this the same color as a prop, so it becomes hard to see that it's a function:
    image
<button @click="fn()" />
  • CON: this just looks like a function is being passed and immidiately executed...
  • CON: if this were a prop, then this way of writing would immidiately execute the function. So it's really confusing
<button @click="() => fn()" />    
  • PRO: it doesn't look like a function is being immidiately executed
  • PRO: it's clear what payload comes out of the click event and what payload goes into the fn, if any

What category should the rule belong to?

[x] Enforces code style (layout)
[ ] Warns about a potential error (problem)
[ ] Suggests an alternate way of doing something (suggestion)
[ ] Other (please specify:)

Provide 2-3 code examples that this rule should warn about:

I believe the explanation above is clear enough but let me know if it doesn't

@mesqueeb mesqueeb changed the title a rule that enforces @click="() => fn()" over @click="fn()" and @click="fn" edit v-on-function-call to enforce @click="() => fn()" over @click="fn()" and @click="fn" Oct 5, 2022
@ota-meshi
Copy link
Member

Thank you for requesting to add the rule.

<button @click="fn" />
<button @click="fn()" />

It seems that rules already exist to check for these.
https://eslint.vuejs.org/rules/v-on-function-call.html

<button @click="fn" />
<button @click="() => fn()" />   

I don't think a rule exists yet to check for these.
Maybe this style can also be used in directives other than v-on, so I think it would be good to add a new rule to enforce the style.

<MyComponent :callback="fn" />
<MyComponent :callback="() => fn()" />   

@mesqueeb
Copy link
Contributor Author

mesqueeb commented Oct 6, 2022

@ota-meshi thank you for your comment.

I feel like it's best that we only keep this rule focused on v-on: because in other directives we might get unexpected side effects.

Eg.
Sometimes I want to immidiately execute a function to pass as a prop:

<button
  :type="calculateType()"
  @click="() => onClick()"
/>

in this case I'd want ESLint to make sure that my @click has the shape of @click="() => onClick()" but the type is just a prop where I'm immidiately executing a function to pass to the prop at runtime.

So I feel it's better to pass a third option to vue/v-on-function-call eg. arrow-function

  "vue/v-on-function-call": ["error", "always"|"never"|"arrow-function"]

OR via a new option

  "vue/v-on-function-call": ["error", "never", { "alwaysPassArrowFunction": true }]

Finally, (but this can be a separate issue)
As per my explanation in the original post, I believe there are only CONs with the first 2 options and only PROs to the last option, so I would even go as far as to suggest in the next major version to make this the default behaviour.

Can anyone can think of any other PROs to the first two versions over the third version as described in my original post?

@FloEdelmann
Copy link
Member

FloEdelmann commented Oct 6, 2022

I agree with @mesqueeb, I don't think checking props is a good idea.
I'd also favor a new option, i.e. "always"|"never"|"arrow-function"; a separate option object is more confusing in this case.

Another CON for the always style: It does only work when there is only one event payload:

$emit('my-event', 'foo', 'bar');

<my-button @my-event="fn" />; // `fn` will be called with both parameters
<my-button @my-event="fn($event)" />; // `fn` will be called with the `foo` parameter only
<my-button @my-event="(foo, bar) => fn(foo, bar)" />; // `fn` will be called with both parameters

Although that may also be seen as an advantage, because one has to comply to that style, i.e. only pass one event parameter everywhere.

A clear PRO for the always and never styles: They are much less verbose than the proposed arrow-function style.

@ota-meshi
Copy link
Member

Thank you for your opinion. I think you are right. It seems I didn't give it enough thought. I think adding options to existing rule is a good idea.

However, I'm not sure if "always"|"never"|"arrow-function" option will meet the user's needs.
In the case below, if the user wants to disallow the second (always style) only, I think the never style and the arrow-function style should be allowed.

<button @click="fn" />
<button @click="fn()" />
<button @click="() => fn()" />

I think we need to use options with arrays to allow multiples.

"vue/v-on-function-call": [
  "error",
  ["never", "arrow-function"] // : ("always"|"never"|"arrow-function")[]
]

What do you think?

@mesqueeb
Copy link
Contributor Author

mesqueeb commented Oct 7, 2022

Yes i think this rule syntax looks good! 🙌🏼

@FloEdelmann
Copy link
Member

Makes sense. To be backwards-compatible, a single option should still be accepted.

@ota-meshi
Copy link
Member

I've started implementing this option, but am worried about enforcing the following style.

<button @click="count++" /> <!-- "always" style -->
<button @click="() => count++" /> <!-- "arrow-function" style -->

I think the new option should report on these stylistic differences as well, but I feel like it's doing more than the rule name.
I think we should add a new rule and deprecate v-on-function-call. I think the name of the new rule should be something like v-on-handler-style.
I think it also renames the options. like "method" | "inline" | "function". (I thought that "arrow-function" was better than "function" when I implemented it. function() {} is also allowed.)
https://vuejs.org/guide/essentials/event-handling.html#inline-handlers
https://vuejs.org/guide/essentials/event-handling.html#method-handlers

What do you think?

@FloEdelmann
Copy link
Member

FloEdelmann commented Oct 10, 2022

Yes, the rule should also check stylistic differences, the old rule name doesn't really convey that. Also, I like that the suggested options method and inline match the Vue documentation. Since there is no equivalent for function in the Vue docs, I'd propose inline-function instead.

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