vee-validate icon indicating copy to clipboard operation
vee-validate copied to clipboard

[TypeScript] Functions which do not use `this` should be typed as arrow functions

Open marlokessler opened this issue 1 year ago • 1 comments

What happened?

Background

In our project, we have the ESLint rule "@typescript-eslint/unbound-method" enabled. This rule warns us, when we use non-arrow functions without their context, because in those functions this might be called, unknowingly from the developer. This happens, e.g. when you destructure an object:

const o = {
  greeting: "Hello World",
  logGreeting() {
    console.log(this.greeting);
  },
};

o.logGreeting(); // Output: _"Hello World"_

const { logGreeting } = o;

logGreeting(); // Output: _undefined_

So to prevent unwrapping functions which are bound to an object, this rule is very helpful.

Issue

However, when destructuring useFieldArray according to the VeeValidate documentation, the same error warning occured. CleanShot 2024-09-13 at 12 21 52

After a look at the source code, I saw that none of the inner functions accessed context with this. So the code worked (of course). However when looking at the type definition of FieldArrayContext I realized, that the functions are declared as normal functions. It's nothing wrong with the definitions, however, normal function types include the assumption that those functions might access context with this and therefore, destructuring is marked as unsafe.

Solution proposal

Since the functions do not access context, arrow function types can be used too:

interface FieldArrayContext<TValue = unknown> {
    fields: Ref<FieldEntry<TValue>[]>;
    remove: (idx: number) => void;
    replace: (newArray: TValue[]) => void;
    update: (idx: number, value: TValue) => void;
    push: (value: TValue) => void;
    swap: (indexA: number, indexB: number) => void;
    insert: (idx: number, value: TValue) => void;
    prepend: (value: TValue) => void;
    move: (oldIdx: number, newIdx: number) => void;
}

This would represent the code more exact, because the functions are also declared independently from a context. This could also be changed for other type declarations too.

Reproduction steps

  1. Setup a project with VeeValidate, ESLint and @typescript-eslint/eslint-plugin
  2. Make sure the rule @typescript-eslint/unbound-method is enabled.
  3. Destructure the returned object of useFieldArray (e.g. const { fields, remove, move } = useFieldArray<SomeType>("path");)
  4. The linting should show a warning

Version

Vue.js 3.x and vee-validate 4.x

What browsers are you seeing the problem on?

  • [ ] Firefox
  • [ ] Chrome
  • [ ] Safari
  • [ ] Microsoft Edge

Relevant log output

No response

Demo link

https://codesandbox.io/p/devbox/friendly-haslett-rqvyc5?workspaceId=88e64305-8239-47e1-bc7e-8e2730ac554d

Code of Conduct

  • [X] I agree to follow this project's Code of Conduct

marlokessler avatar Sep 13 '24 11:09 marlokessler

Interesting, I wasn't aware this would affect someone. Thanks for raising this. Do you think you can PR it?

logaretm avatar Sep 13 '24 11:09 logaretm