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.