fix: use arrow functions over bound funcs

[aloisklink]

Enable the ESLint @typescript-eslint/unbound-method-error rule and fix/ignore all the errors it throws at us.

Basically, this rule warns us from unbinding a function (e.g. doing const {x} = {x() { /* etc */}} that doesn't explicitly have this: void.

I've replaced these types with arrow functions when possible, to avoid users of the SvelteKit library from facing the same ESLint issues.

There are some instances where ESLint throws an error when we are purposely doing this (e.g. we'd later call func.apply(originalThis, or func.call(originalThis,, but since they're rare, I've just added // eslint-disable-next-line @typescript-eslint/unbound-method -- We'll pass `xxxxx` as `this` comments there to ignore the error.

kit/packages/kit/src/runtime/client/client.js

Lines 91 to 95 in b84e4dc

	// eslint-disable-next-line @typescript-eslint/unbound-method -- We'll pass `history` as `this`
	const push_state = history.pushState;
	history.pushState = (...args) => {
	warn();
	return push_state.apply(history, args);

I've also manually fixed some types in packages/kit/src/exports/public.d.ts that weren't caught by the @typescript-eslint/unbound-method-error rule, since they're only destructured in packages/kit/test/apps, which is ignored by our ESLint config. However, this manual change should fix #12622.

---

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 for me, but since the @typescript-eslint/* rules ignore packages/kit/test/apps/**, it's possible that I've missed some issues.
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.
  • ESLint fails before most of these changes.

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:.
  • The changes in the packages/package should be internal only, so we don't need to make a changeset for it.

Edits

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

[changeset-bot]

🦋 Changeset detected

Latest commit: ec8dedc

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

[aloisklink]

GitHub Actions is having issues: https://www.githubstatus.com/incidents/9yk1fbk0qjjc Edit: Resolved

[benmccann]

Does this actually fix any bugs? It mostly looks like busy work to me

[Stadly]

I think it will close sveltejs/svelte#13481 and #12622

[benmccann]

I see. So this would primarily help people who want to use that eslint rule in their own projects. I think what I might prefer to do is accept the changes to update the type definitions, but remove the enforcement and eslint ignore comments. One the pattern is in place, we'll follow it to be consistent, but I'd rather not litter the code with eslint ignores if we're not also avoiding bugs in the SvelteKit code

[aloisklink]

I see. So this would primarily help people who want to use that eslint rule in their own projects.

Yep! It's part of the recommended-type-checked config of TypeScript ESLint, so https://typescript-eslint.io/rules/unbound-method/

I think what I might prefer to do is accept the changes to update the type definitions, but remove the enforcement and eslint ignore comments.

Sounds good to me! Although I might make a separate PR for that instead, since it's quite different from this PR (we can always close this PR later). Edit: Made in #12955

One the pattern is in place, we'll follow it to be consistent, but I'd rather not litter the code with eslint ignores if we're not also avoiding bugs in the SvelteKit code

Although, every // eslint-ignore is either because:

• The types are wrong in @types/cookie, but are fixed in cookie v1.0.0, e.g.

  kit/packages/kit/src/runtime/server/cookie.js

  Lines 166 to 167 in ec8dedc

  	// eslint-disable-next-line @typescript-eslint/unbound-method -- Fixed in `[email protected]`'s types
  	const encoder = cookie.options.encode || encodeURIComponent;

• We're purposely unbounding functions, but we'll call the functions with .apply(actualThis), e.g. like

  kit/packages/kit/src/runtime/client/client.js

  Lines 98 to 103 in ec8dedc

  	// eslint-disable-next-line @typescript-eslint/unbound-method -- We'll pass `history` as `this`
  	const replace_state = history.replaceState;
  	history.replaceState = (...args) => {
  	warn();
  	return replace_state.apply(history, args);
  	};

  (mainly because we're monkeypatching something)
  The TypeScript ESLint docs do say something about this (see https://typescript-eslint.io/rules/unbound-method/#when-not-to-use-it):

  When Not To Use It

  If your project dynamically changes this scopes around in a way TypeScript has difficulties modeling, this rule may not be viable to use. For example, some functions have an additional parameter for specifying the this context, such as Reflect.apply, and array methods like Array.prototype.map. This semantic is not easily expressed by TypeScript. You might consider using ESLint disable comments for those specific situations instead of completely disabling this rule.

This ESLint rule could help prevent bugs, but most of these invalid scope bugs would probably be caught in SvelteKit's tests, so it's probably not needed.

[aloisklink]

I've made a separate PR that just modifes the public types and doesn't enable the ESLint @typescript-eslint/unbound-method-error rule in #12955.

Once it's merged, we can either decide to:

• keep this PR (although I'll have to remove the Changeset since it'd be a refactoring/internal-only change), or
• close this PR (maybe we can mark it as #wontfix).