fix: update onMount type definition to prevent async function return

[chrskerr]

Breaking change / how to migrate

The onMount type definition was updated to make returning a function (or any, which could also be a function) asynchronously is a type error because in 90% of cases it hints at a mistake: You likely want the returned function to be called on destroy, but that does not happen for asynchronously returned functions.

To migrate, adjust the code so you don't return a function asynchronously. Note that you also get the error if you return any because any includes function types.

If this change revealed an actual bug in your code:

onMount( - async () => { - const something = await foo(); + () => { + foo().then(something => .. // .. return () => someCleanup(); } );

If this change results in a false positive, just make it not return a function:

- onMount(() => someFunctionThatReturnsAFunctionButYouDontCare()) + onMount(() => { someFunctionThatReturnsAFunctionButYouDontCare(); })

PR description

From the docs for onMount (https://svelte.dev/docs#run-time-svelte-onmount) a cleanup function can be returned from onMount which will then be used on dismount for cleanup. These only work for synchronous functions, not async.

We just discovered a few mistakes in our codebase in which the following pattern was used:

onMount(async () => { // do mount return () => { // cleanup } })

I am proposing a types change to the onMount function to only accept the following function signatures:

• () => void - async no cleanup
• () => Promise<void> - async no cleanup
• () => (() => void) - synchronous cleanup

This will catch mistakes where the cleanup would not be run.

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
• Prefix your PR title with [feat], [fix], [chore], or [docs].
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

• Run the tests with npm test and lint the project with npm run lint

[dummdidumm]

Makes sense!

[baseballyama]

Suggested change

	export function onMount(fn: (() => () => void) | (() => void | Promise<void>)) {
	export function onMount(fn: (() => () => any) | (() => any | Promise<any>)) {

In this case, this will be a type error, right?
But fundamentally, there is no problem.

// api.js export const fetch = () => Promise.resolve({foo: 'bar'}); // Component.svelte <script>  import { onMount } from 'svelte';  import { fetch } from './api';  onMount(() => fetch()) </script>

So IMO, it's better to use any type instead of void.

[chrskerr]

Thanks for the suggestion but I think that this won't work to prevent a function return from an async onMount.

I am attempting to cause a typescript failure on arguments of type () => Promise<() => any>, but the introduction of () => any means that this remains a valid argument.

Given I only actually want to disallow this single type, perhaps the following is more appropriate:

type AnyExcept<Type, Disallowed> = Type & (Type extends Disallowed ? never : Type); export declare function onMount<T>(fn: () => AnyExcept<T, Promise<() => any>>) {

Testing locally, this allows your example above and any other return from onMount except for the following:

onMount(async() => { // fails  return () => { /// anything } })

[vercel]

@baseballyama is attempting to deploy a commit to the Svelte Team on Vercel.

A member of the Team first needs to authorize it.

[baseballyama]

I updated the type.
Now type check works like this.
Is it correct?

<script lang="ts">  import {onMount} from 'svelte';   // sync and no return (NO TYPE ERROR)  onMount(() => {  console.log("mounted");  });   // sync and return value (NO TYPE ERROR)  onMount(() => {  return 'done';  });   // sync and return sync (NO TYPE ERROR)  onMount(() => {  return () => {  return "done";  };  });   // sync and return async (NO TYPE ERROR)  onMount(() => {  return async () => {  const res = await fetch();  return res;  };  });   // async and no return (NO TYPE ERROR)  onMount(async () => {  await fetch();  });   // async and return value (NO TYPE ERROR)  onMount(async () => {  const res = await fetch();  return res;  });   // async and return sync (**TYPE ERROR**)  onMount(async () => {  return () => {  return "done";  };  });   // async and return async (**TYPE ERROR**)  onMount(async () => {  return async () => {  const res = await fetch();  return res;  };  }); </script>

[dummdidumm]

I added tests so we can check against regressions later. During that I noticed one case where there's now an error, which I'd like to discuss if this is a blocker from merging this, or if it's ok: If you return any from onMount, it's an error.

onMount(async () => { const a: any = null as any; return a; });

How likely is it that this happens? This probably only happens in the wild by accident through something like onMount(() => someAsyncFunctionWhichReturnsAny()). How often is that? Are we ok with it?

I'd be ok with this. Please vote thumbs up/down if you think this is ok/not ok.

[Conduitry]

If this is just types and is not a runtime check for something that looks like a promise, I think this is fine. If someone really has a good/weird reason for going that, they can add a ts-ignore.