Enable .then with event dispatchers

[AyloSrd]

Describe the problem

When using an event dispatcher generated with createEventDispatcher() we may need, once the callback function is executed, to chain another callback, or another event. This is especially relevant when the event callback is an asynchronous one (e.g. an API call); as of now, we can achieve this only by notifying the child component that the first callback has been executed via the props, and then react to the notification with another callback ; it would be great to being able to use .then directly with the event dispatcher, like this :

<script>
    import { createEventDispatcher } from 'svelte'
    
    const dispatch = createEventDispatcher()

    function showOkMessage(){
        // some async logic
        // this function would show for some seconds
        // an 'ok the data was fetched' message
    }
</script>

<button 
    on:click={() => dispatch('makeAsyncCall').then(showOkMessage).then(() => dispatch('close'))}
>
    fetch
</button>

Describe the proposed solution

A possible implementation would be something like this (disclaimer: I haven't tested it yet, it probably needs to be refined to work):

function createEventDispatcher() {
	const component = current_component;

	return (type, detail) => {
		const callbacks = component.$$.callbacks[type];

		if (callbacks) {
			let arr = []
			const event = custom_event(type, detail);
			callbacks.slice().forEach((fn) => {
				const res = fn.call(component, event);
				if (res instanceof Promise) { 
					arr = [...arr, res]
				} else {
					const asyncRes = new Promise(resolve => resolve(res))
					arr = [...arr, asyncRes]
				}
			});
			return Promise.all(arr)
		}
	};
}

Alternatives considered

Alternatively, the returned promises could carry some sort of payload; although this wouldn't be clean, because it could lead to use cases that go against the 'one source of truth' principle.

Importance

nice to have

[rmunn]

At first glance, I like the idea as it provides more flexibility. Being able to find out when event handlers have finished running has parallels with the DOM event model: https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/dispatchEvent says that "All applicable event handlers are called and return before dispatchEvent() returns."

I do have a couple comments on your proposed implementation. The first is that you don't have a return value if the callbacks value is null or undefined; you'd want to add an else condition to the if (callbacks) {} block that would return an already-resolved promise, to guarantee that createEventDispatcher() always returns a promise that can be awaited.

And second, while I appreciate the fact that you prefer to build up arrays in immutable-data fashion using arr = [...arr, newValue ], in this case that's not actually needed since no outside code is ever given access to the array. So you can use arr.push(res) and arr.push(asyncRes) instead, which is going to be more efficient. Not that it will matter most of the time, since the difference between O(N) and O(N^2) only starts to matter when N gets large and most Svelte events aren't going to have that many handlers. But if you can gain efficiency without losing readability, it's usually a good idea to do so.

Overall, though, I like this idea. The only drawback I can think of is that people might be tempted to use .then() chaining to pass messages from parent to child that really belong in props. (As you said, going against the "one source of truth" principle). Promise.all() does, after all, return an array of the values the promises resolved to. But since the child component (which dispatched the event) will have no way of knowing what order the handlers ran, if it's looking for a particular value from a parent it won't be able to know which index that value is in. So that will probably discourage people from abusing the return values.

[AyloSrd]

@rmunn , thanks for the feedback, you made two excellent points.
I believe the new implementation below (that seems to work in this REPL) is in line with your suggestions:

function createEventDispatcher() {
	const component = get_current_component();
	return (type, detail) => {
		const callbacks = component.$$.callbacks[type];
		if (callbacks) {
			let arr = []
			const event = custom_event(type, detail);
			callbacks.slice().forEach((fn) => {
				const res = fn.call(component, event);
				if (res instanceof Promise) arr.push(res)
			});
			return Promise.all(arr).then(() => true)
		}
	};
}

I realized that I don't need to push all the the callbacks into the array of Promises, as the synchronous ones (if I am not wrong) will be executed before the end of each iteration, so I don't have to artificially wait for them by wrapping them into a Promises (same applies to null and undefined).
Also, after reading the doc you provided (here), I made the dispatcher return a Promise that resolves in true. In this way, we achieve two things:

1. it sticks to the parallel with the DOM's dispatchEvent, which should return true unless one of the handlers called e.preventDefault()
2. it avoids returning values that could be used to infringe the 'one source of truth' principle

[rmunn]

Looks good. You still need an else clause on the if (callbacks) block that will return an already-resolved promise, to ensure that you can never end up with a TypeError as client code tries to call undefined.then(doSomething).

[AyloSrd]

Oh yes, I understand now.
Sure, I agree, but probably I could just put another return after the if block; no need to wrap it in an else block, because it will be returned just if the if block doesn't execute.

This raises another question though, should this second Promise resolve to true, to false, or reject it with some sort of 'no handler for this event' error ?
(It should probably resolve to false, as rejecting will require for the user to always use .catch)

function createEventDispatcher() {
	const component = get_current_component();
	return (type, detail) => {
		const callbacks = component.$$.callbacks[type];
		if (callbacks) {
			let arr = []
			const event = custom_event(type, detail);
			callbacks.slice().forEach((fn) => {
				const res = fn.call(component, event);
				if (res instanceof Promise) arr.push(res)
			});
			return Promise.all(arr).then(() => true)
		}
		return new Promise((resolve) => resolve(false))
	};
}

[rmunn]

Well, currently there's no equivalent to event cancellation for Svelte-dispatched events, so there are no circumstances in which you'd need to return false to parallel the way HTML's dispatchEvent works. So I see two possibilities. One is to always return true since the event wasn't cancelled (in which case the return value is meaningless and you might as well return undefined as return true). The other is to return true if there were any handlers and false if there were no handlers. The latter makes the most sense, IMHO. And that's almost what you've written, but you don't handle the case where the callbacks array exists but is empty. (Which can happen). What you'd need would be something like this:

		if (callbacks) {
			let arr = []
			const hasCallbacks = !!callbacks.length
			const event = custom_event(type, detail);
			callbacks.slice().forEach((fn) => {
				const res = fn.call(component, event);
				if (res instanceof Promise) arr.push(res)
			});
			return Promise.all(arr).then(() => hasCallbacks)
		}
		return new Promise((resolve) => resolve(false))

Then the result would be false in all cases where there were no callbacks, and could be relied on in a case like dispatch(event).then(handled => { if (!handled) console.log('Nobody was listening'); }).

[AyloSrd]

I agree with you, the latter makes most sense.
Thank you very much for your help!
I'll wait some days to see if someone will PR it, and if not I'll try to do it myself.

[fartinmartin]

This is a sweet idea! I tried implementing it in a project which was forwarding my custom events and it breaks, unfortunately. I'm not smart enough to figure out why at the moment, but here's a REPL, and potentially a place to find answers?

[AyloSrd]

@fartinmartin thx!
I've managed to make it work and have almost finished the tests, I will try to push a PR by the end of the weekend.

[fartinmartin]

@AyloSrd incredible! Excited to see your PR! 😊

[bluwy]

I'm not really sure this is the right feature to add. Event dispatchers (and the event model in general) is sync only, similar to DOM events, once dispatched it doesn't wait for any promises and returns immediately.

For example,

const a = document.getElementById('anchor_tag')
a.addEventListener('click', async (e) => {
  await doSomething()
  e.preventDefault() // stop browser from opening the link (does not work)
})

The code above doesn't work as e.preventDefault() needs to be called synchronously.

---

While Svelte's internal implementation allows bypassing this caveat, I think it's important for Svelte to map the DOM's limitation to prevent gotchas for end-users down the road.

[AyloSrd]

Hi @bluwy thx for your feedback, it's a valid point.

Please see the reasoning behind my feature request, that focuses on two points:

1. The aforementioned parallel with the DOM: even if, as you rightly point out, DOM dispatchers are synchronous, they still return after all the handlers have returned, thus letting know when the latter have run, as pointed out by @rmunn here. Also event dispatchers return a boolean, depending on whether at least one of the handlers has called e.preventDefault(). Svelte's dispatchers don't return anything, and, given the nature of the custom events they create, it wouldn't make sense to check if any event has been cancelled. Thus, I believe it still makes sense to notify when handlers have run, and, since async operations are those for which it makes more sense being notified, it'd be better if the notification comes in a form of a promise. Also, please consider how this feature would only add on top of DOM current behaviour, instead of replacing or modifying it (in fact, while with the requested feature we would wait for the handlers to resolve before resolving in true or false, we would still return a promise just right after the handlers have returned).

2. The Component-based nature of Svelte: ideally a component should be able to behave almost like a native element, in the sense that the parent element should treat it as it would a native one, being agnostic of it's children's internal working. Likewise, children elements should be conceived independently from their potential parents. Knowing when synchronous and asynchronous handlers have returned/resolved will, IMO, help make components much more independent, giving a lot more flexibility to the developer and improving dev-xp by removing the need of passing additional props. This Component-nature reason would justify taking this slight detour from classical DOM dispatchers. Furthermore, Svelte's dispatcher has already some important differences with the DOM's one, e.g. allowing passing a payload, while in vanilla JS it should be passed during the event initialisation (this is because Svelte's dispatch also create the custom event). Therefore I don't believe that adding this feature would negative impact or shock the end user, as it has already been made clear that the two dispatch are not perfectly aligned.

I hope my points are clear enough.

To be more faithful to DOM behaviour, I could avoid waiting for the handlers to resolve, and only return true when they return, or false when the handler is null or undefined . What would be your opinion/suggestions?
Personally, I believe it wouldn't add as much flexibility as the async implementation I suggested above, but I'm very open to any suggestions.
Thx in advance

[bluwy]

Svelte's dispatchers don't return anything, and, given the nature of the custom events they create, it wouldn't make sense to check if any event has been cancelled.

It does return a boolean since 3.48.0 (#7064), aligning with the DOM dispatchEvent API.

Also, please consider how this feature would only add on top of DOM current behaviour, instead of replacing or modifying it (in fact, while with the requested feature we would wait for the handlers to resolve before resolving in true or false, we would still return a promise just right after the handlers have returned).

I believe this is the main concern, we shouldn't be adding features on top of the reflecting DOM API, and instead try to replicate it as close as possible.

Knowing when synchronous and asynchronous handlers have returned/resolved will, IMO, help make components much more independent, giving a lot more flexibility to the developer and improving dev-xp by removing the need of passing additional props.

While I think this is true, deviating from the DOM API isn't the right solution. It would probably take more boilerplate to support your usecase, but I think it's more correct so we don't break standards.

Furthermore, Svelte's dispatcher has already some important differences with the DOM's one, e.g. allowing passing a payload, while in vanilla JS it should be passed during the event initialisation (this is because Svelte's dispatch also create the custom event)

I don't see how Svelte's is different than the DOM. For me:

dispatch('my-event', { pay: 'load' }, { cancelable: true })

maps to

el.dispatchEvent(new CustomEvent('my-event', { detail: { pay: 'load' }, cancelable: true })

Svelte's simply providing sugar to dispatch events within its own restrictions (components), and it doesn't implement anything non-standard. Perhaps you can elaborate more on something that I've missed?

[AyloSrd]

It does return a boolean since 3.48.0 (#7064), aligning with the DOM dispatchEvent API.

Oh, I completely missed this part! My bad.
I opened this issue in November, and only recently started working on it, but I just assumed the code hadn't changed, sloppy move on my part 🙈.
In that case, I guess my previous point 1 is overdated, and in any case implementing my feature request would break any backwards compatibility, so I guess I can safely close this issue.
Thx for pointing it out @bluwy !

[bluwy]

No problem. I should've brought up that PR earlier 😅 which seems like indeed would cause a breaking change otherwise. I'll close this issue then. Thanks for considering some of the points in the conversation!

[AyloSrd]

@fartinmartin btw, here a REPL for you to see how it would have been implemented, code-wise.