fix: use arrow function types over bound functions

[aloisklink]

Using class method function types causes SvelteKit users to get TypeScript ESLint errors if they're using the @typescript-eslint/unbound-method rule (included by default in the recommended-type-checked config).

For example:

/home/projects/stackblitz-starters-1y7qpk/src/routes/sum/+layout.ts
  3:42  error  Avoid referencing unbound methods which may cause unintentional scoping of `this`.
If your function does not access `this`, you can annotate it with `this: void`, or consider using an arrow function instead  @typescript-eslint/unbound-method

Switching SvelteKit's public types to arrow functions fixes #12622.

Implementation decisions

I haven't enabled the @typescript-eslint/unbound-method rule in this PR. However, I've got a separate PR (#12915) where we enable this rule and we fix internal types as well.

I kept the following types in public.d.ts using non-arrow functions, since:

• These types are for user inputs, which might rely on these functions being bound, and so it might be a breaking change to change these types:
  • Emulator['platform']
  • functions in KitConfig
• Class methods that rely on this:
  • Server

I also had to make some changes to packages/kit/src/runtime/server/cookie.js to make TypeScript happy.

---

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Fixes parent in load functions receive @typescript-eslint/unbound-method error #12622
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.
  • No tests, however fix: use arrow functions over bound funcs #12915 enables the @typescript-eslint/unbound-method rule as well and does test this.
    However, I've split it up on the advice of @benmccann (see fix: use arrow functions over bound funcs #12915 (comment))

Tests

• Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

• If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Edits

• Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

[changeset-bot]

🦋 Changeset detected

Latest commit: 743d807

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@sveltejs/kit	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[eltigerchino]

After a quick find operation on the public.d.ts file with the keyword ): for bound functions, there are are still 28 other functions using the bound function syntax. Should we change all of them to arrow functions for the sake of standardisation? cc: @benmccann

[aloisklink]

After a quick find operation on the public.d.ts file with the keyword ): for bound functions, there are are still 28 other functions using the bound function syntax. Should we change all of them to arrow functions for the sake of standardisation?

I'm for it, but I was a bit reluctant since it's difficult to know which functions should be bound and can't be safely destructured. So instead I only changed the places that the @typescript-eslint/unbound-method ESLint rule caught.

If you look at my original PR (#12915) where I've also enabled the @typescript-eslint/unbound-method ESLint rule, you can see there are a couple of cases where I've added // eslint-disable-next-line @typescript-eslint/unbound-method, since we are unbinding functions that do use the this arg.

Unfortunately, I don't think there's an easy ESLint rule to ensure you stay on one standard or the other, other than potential TypeScript/@typescript-eslint/unbound-method errors if you use the wrong style.

[Stadly]

After a quick find operation on the public.d.ts file with the keyword ): for bound functions, there are are still 28 other functions using the bound function syntax. Should we change all of them to arrow functions for the sake of standardisation? cc: @benmccann

I think maybe the correct thing to do is to inspect the function definitions and use arrow function for functions that don't reference this and the bound function syntax for functions that reference this.

[benmccann]

I'm not much of a TypeScript expert, but that sounds reasonable to me

[Stadly]

Are you able to look into it, @aloisklink?

[aloisklink]

In 743d807, I've updated the rest of the public.d.ts file to use arrow functions when possible.

I kept the following types in public.d.ts using non-arrow functions, since:

• These types are for user inputs, which might rely on these functions being bound, and so it might be a breaking change to change these types:
  • Emulator['platform']
  • functions in KitConfig
• Class methods that rely on this:
  • Server

I also had to make some changes to packages/kit/src/runtime/server/cookie.js to make TypeScript happy.

---

Also, I found #7396 which did the opposite of this! I still think arrow functions are better so that users can destructure the functions from objects when necessary, but @ignatiusmb (the author of that PR) brought up some reasons why the current function syntax is better (maybe it could be used internally?).

---

Are you able to look into it, @aloisklink?

Sorry for the late update! I wanted to get it done last weekend, but I didn't get the time!