Skip to content

feat(runtime-core): return result of handlers in emit#6819

Open
jaulz wants to merge 4 commits intovuejs:mainfrom
jaulz:patch-5
Open

feat(runtime-core): return result of handlers in emit#6819
jaulz wants to merge 4 commits intovuejs:mainfrom
jaulz:patch-5

Conversation

@jaulz
Copy link
Copy Markdown
Contributor

@jaulz jaulz commented Oct 4, 2022

It would be great if emit could return the result of the handlers. A typical example could be to show a loading indicator while asynchronous event handlers are running:

const loading = ref(false)

const handleClick = async () => {
  loading.value = true
  await Promise.all(emit('click'))
  loading.value = false
}

This is just a basic example that does not consider errors or multiple clicks but it gives an idea what would be possible. The same could already be implemented with props but it duplicates the handling.

If you think this is a good idea, I can extend the PR with corresponding tests.

Copy link
Copy Markdown
Member

@sxzz sxzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if we should have an issue discussing this feature first.

}
instance.emitted[handlerName] = true
callWithAsyncErrorHandling(
result = callWithAsyncErrorHandling(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If handler and onceHandler are existing at the same time, the result will be overwritten.

return compatInstanceEmit(instance, event, args)
}

return result
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should change type declaration too

export type EmitFn<
Options = ObjectEmitsOptions,
Event extends keyof Options = keyof Options
> = Options extends Array<infer V>
? (event: V, ...args: any[]) => void
: {} extends Options // if the emit is empty object (usually the default value for emit) should be converted to function
? (event: string, ...args: any[]) => void
: UnionToIntersection<
{
[key in Event]: Options[key] extends (...args: infer Args) => any
? (event: key, ...args: Args) => void
: (event: key, ...args: any[]) => void
}[Event]
>

@jaulz
Copy link
Copy Markdown
Contributor Author

jaulz commented Oct 5, 2022

I'm not sure if we should have an issue discussing this feature first.

Yep, for sure, I thought this PR could serve as an initial base to discuss it further. Happy to move it somewhere else if you want?

@netlify
Copy link
Copy Markdown

netlify Bot commented Oct 5, 2022

Deploy Preview for vuejs-coverage failed.

Name Link
🔨 Latest commit a1cac63
🔍 Latest deploy log https://app.netlify.com/sites/vuejs-coverage/deploys/633d2b62989f1b000874005a

instance.emitted = {} as Record<any, boolean>
} else if (instance.emitted[handlerName]) {
return
} else if (!instance.emitted[handlerName]) {
Copy link
Copy Markdown
Member

@sxzz sxzz Oct 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If !instance.emitted is true, it will skip else.

Suggested change
} else if (!instance.emitted[handlerName]) {
} if (!instance.emitted[handlerName]) {

const results = []
if (handler) {
callWithAsyncErrorHandling(
const values = callWithAsyncErrorHandling(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if values could be an array? If handler is not an array, then values is not.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the definition of callWithAsyncErrorHandling it can return an array:

export function callWithAsyncErrorHandling(
fn: Function | Function[],
instance: ComponentInternalInstance | null,
type: ErrorTypes,
args?: unknown[]
): any[] {
if (isFunction(fn)) {
const res = callWithErrorHandling(fn, instance, type, args)
if (res && isPromise(res)) {
res.catch(err => {
handleError(err, instance, type)
})
}
return res
}
const values = []
for (let i = 0; i < fn.length; i++) {
values.push(callWithAsyncErrorHandling(fn[i], instance, type, args))
}
return values
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends on handler. If handler is a function, then callWithAsyncErrorHandling will return the result directly. I don't think handler will be an array.

@newVincentFong
Copy link
Copy Markdown

It's an underrated feature here!
First of all, it would really be nice to have:

<!-- Outside -->
<SomeFormComponent
	@validate="validateData"
	@submit="saveDataToDatabase"
/>
<script>
async function validateData() {
	await API.validateXXX()
}
async function saveDataToDatabase() {
	await API.postXXX()
}
</script>
<!-- Inside -->
<form />
<script>
const emit = defineEmits()
async function handleClickSubmit() {
	startLoading()
	await emit('validate', data)
	await emit('submit', data)
	stopLoading()
}
</script>

Nice and clean, right?
But for now, we can't write in this way.
Because there is actually an untold rule on event handlers.

defineEmits<{
	(e: 'submit'): Promise<undefined> // ❌
	(e: 'submit'): void // ✅
}>()

The return type can only be void.
You won't get the result returned by your event handlers.
The whole EMIT thing is basically an EventEmitter, and in a typical EventEmitter model, one event key can have multiple handlers.
Despite the fact that @sxzz has looked up the code, and handler won't be an array.
So, Vue should make a decision on its Emit model.
Either Vue supports Returning result of event handlers or stress the void type in doc like:

defineEmits<{
	(e: 'aaa'): void
	(e: 'bbb'): void
	/** .... */
}>()

@sxzz sxzz requested a review from yyx990803 November 10, 2022 10:17
@mrchar
Copy link
Copy Markdown

mrchar commented May 8, 2023

I realy need this feature, is this feature likely to be merged? when will it be available?

@skirtles-code
Copy link
Copy Markdown
Contributor

There's an RFC discussion at vuejs/rfcs#587 that proposes a slightly different way of approaching this. I'm not sure which I prefer.


Just to clarify, a single event can have multiple listeners. In the Playground below, the emitted event triggers 3 handlers. mergeProps ensures that these are combined into an array.

emit will need to return an array in all cases, it can't just assume a single listener. The RFC above seems to be suggesting that the array should be automatically wrapped in a promise, whereas the approach here seem to leave it to the user to call Promise.all.

@edison1105
Copy link
Copy Markdown
Member

I prefer the proposal in this PR to vuejs/rfcs#587. This implementation is more flexible, giving the user control over exactly what is returned, rather than just returning a boolean value.

@edison1105 edison1105 added ✨ feature request New feature or request need guidance The approach/solution in the PR is unclear and requires guidance from maintainer to proceed further. labels Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

need guidance The approach/solution in the PR is unclear and requires guidance from maintainer to proceed further. ✨ feature request New feature or request

Projects

Development

Successfully merging this pull request may close these issues.

6 participants