clarify onMount return functions when combined with non-synchronous handlers

[antony]

Fixes #4927

Before submitting the PR, please make sure you do the following

• It's really useful if your PR relates to an outstanding issue, so please reference it in your PR, or create an explanatory one for discussion. In many cases features are absent for a reason.
• This message body should clearly illustrate what problems it solves. If there are related issues, remember to reference them.
• Ideally, include a test that fails without this PR but passes with it. PRs will only be merged once they pass CI. (Remember to npm run lint!)

Tests

• Run the tests tests with npm test or yarn test)

[Conduitry]

I do think we ought to clarify to people here how this works, but I'm not sold on this particular wording. Maybe instead just saying something about how 'the function passed to onMount needs to synchronously return a function for it to be called when the component is destroyed'? And then maybe a reminder that async functions always actually return promises.

[antony]

Updated. It's very hard to get the wording right in a concise way without requiring the user to have a very solid grounding in asynchronous functions, anonymous functions, and Promise execution order.

I think I've covered it though.

[Conduitry]

Do we want to maybe say "returns a function" rather than "returns a value"? The only thing I'd worry with about that is the ends of the two sentences sounding a bit repetitive.

[antony]

The reason I've said value in the first instance, is that I wouldn't consider this:

const myOnDestroy = function () { disposeStuff() } 
onMount(async () => new Promise(r => r(myOnDestroy))

as returning a function, more returning a value. I'd want to cover people literally using promises as return values within async functions.

The second sentence refers to a concrete example.

[Conduitry]

That's fair. This seems like a good explanation then, and there'll at least be some place in the docs to point people to about this now.

[Leftium]

The note in the docs finally helped me solve the issue, but it would be nice if Svelte gave some warning when this happens.

This issue took 10+ hours to debug in my project. I did not realize new event handlers were being created without destroying the old ones. So events triggered both the old and new event handlers, causing weird behavior.

I almost attributed the weird behavior to something completely unrelated (my first fix related to Svelte stores turned out to be just a work-around).

This PR also seems like it would have helped: #8136