Skip to content

Always use return in wrapped method #1410

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 6, 2022
Merged

Conversation

leaanthony
Copy link
Member

Fixes #1408

@leaanthony leaanthony marked this pull request as ready for review May 19, 2022 10:47
@cloudflare-workers-and-pages
Copy link

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: fdc055e
Status: ✅  Deploy successful!
Preview URL: https://48d9bd20.wails-website.pages.dev

View logs

Copy link
Contributor

@ianmjones ianmjones left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I was worried that it would undo the changes in #1356, but by returning void and not a void promise we get the synchronous functionality that #1408 wants back, without breaking #1355.

Nice one!

@leaanthony
Copy link
Member Author

Now that you've said that, I'm wondering if this does satisfy both scenarios. One wants to know when the method ends and the other doesn't care. The TS can't satisfy both (or can it 🤔 could we return <Promise<void>|void>)

I'm wondering whether method calls should always be async, and events should be used for "fire and forget" scenarios. That was the original intent of both and perhaps that should have been the solution to the original request. Thoughts?

@ianmjones
Copy link
Contributor

I'm wondering whether method calls should always be async...

You mean, always return a promise again?

If so, then I personally don't think that's a good idea, but I'm happy to be convinced otherwise.

I feel like it's a nice set up at the moment with if there is something to be returned, and the last return item is an error, then you get a promise with the return item, and catch the error.

If there is only an error in the return, I take it there's still a void promise with a possible catch?

If there is nothing to return, I personally think the previous async nature was fine as there's no result to process, and therefore nothing to await for (or chain with then). However, this new setup where you can await if you want to is fine too.

..., and events should be used for "fire and forget" scenarios.

I haven't looked at the events side of things yet, but I assumed they had no expectation of a return and so were fire and forget. I assumed they were for "this thing just happened with these details", meaning you could not change the outcome of what triggered the event, only react to it.

That was the original intent of both and perhaps that should have been the solution to the original request. Thoughts?

I hope I interpreted your intents correctly.

@ianmjones
Copy link
Contributor

I've slept on it, and I do wonder whether events should be synchronous or not. Could you have events that expect a response?

One of my favourite things about WordPress is its hook system (this may be an unpopular opinion). Fundamentally there are two ways to fire off a hook, as an action or filter, both are synchronous.

An action just says "this thing is will or did happen", and has no return.

do_action( 'example_action', $arg1, $arg2 );

While at first glance you may think, ok, this could just be async then as there is nothing returned and the emitter doesn't care. In reality, an action gives other parts of the system a chance to prepare for the event that will happen whether they like it or not. Or deal with the results of an action, before the framework continues with post action stuff.

These actions in WP are like the events in Wails at the moment as they do not expect anything back when emitted.

A filter gives none or many filter handlers a chance to alter a value before it is used.

$value = apply_filters( 'example_filter', 'filter me', $arg1, $arg2 );

These are clearly synchronous as the emitter is waiting for all filter handlers to finish taking a look at the value to see if they want to change it before it then uses the value.

Filters are probably the most used type of hook, and quite often used for "hey, should I proceed with this action" kinds of things...

$cancel = apply_filters('about_to_do_thing', false, $context );

Wails doesn't have this kind of event, yet (maybe v3?).

So on reflection, it does seem like events should be emitted synchronously as the receiver may or may not want to prepare for whatever the event signifies is about to happen, or has happened. There's nothing to stop the event receiver from taking note of the event and then asynchronously firing off a process to deal with it, e.g. logging, or some long running task.

Sorry for the long post, but you did ask for thoughts! 😂

@leaanthony
Copy link
Member Author

Am away for a week so interested in all thoughts on this!

@leaanthony leaanthony merged commit fe34174 into master Jun 6, 2022
@leaanthony leaanthony deleted the bugfix/promise_returns branch June 6, 2022 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[V2] void function need to return Promise
2 participants