Skip to content
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

Add advertiseAsync method to Service #698

Merged
merged 2 commits into from
Mar 19, 2024

Conversation

EzraBrooks
Copy link
Contributor

@EzraBrooks EzraBrooks commented Mar 18, 2024

Public API Changes

Adds an advertiseAsync function to the Service class to allow Promise-returning functions as callbacks to services.

Description

☝🏻
This is useful for cases where, like a feature I'm working on right now in my application, you want to wait to return a Service call until a different asynchronous operation completes. The original advertise function is very limiting because it requires all operations undertaken by the Service server to be synchronous due to the design of asynchronicity in JavaScript disincentivizing synchronizing async closures into synchronous ones.

vite.config.js Outdated Show resolved Hide resolved
Comment on lines +135 to +137
if (this.isAdvertised) {
throw new Error('Cannot advertise the same Service twice!');
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should move toward this behavior in all of the cases throughout the library where there's currently guard conditions that just early-return with no information. Users shouldn't be calling advertise multiple times on the same Service object, that's probably an issue with their code!

I don't want to break that in existing functions yet, but figured I'd do it here where we have a green field.

Copy link
Contributor

Choose a reason for hiding this comment

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

We have not released any v2 version yet. So I think we can fix it everywhere.

@EzraBrooks EzraBrooks force-pushed the add-async-form-to-service-advertisement branch from 95dab35 to 00b945c Compare March 18, 2024 17:01
Copy link
Contributor

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

very nice!

Copy link
Contributor

@MatthijsBurgh MatthijsBurgh left a comment

Choose a reason for hiding this comment

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

Do we to add unadvertise functionality?

@EzraBrooks
Copy link
Contributor Author

I believe the existing unadvertise should work here - note that the existing unadvertise doesn't actually remove the event handler from the EventEmitter, so no changes are needed to make this behave the same. That's actually probably a bug - maybe worth fixing for both the synchronous and asynchronous version at the same time.

@MatthijsBurgh
Copy link
Contributor

That's actually probably a bug - maybe worth fixing for both the synchronous and asynchronous version at the same time.

@EzraBrooks sounds like a good idea

@EzraBrooks
Copy link
Contributor Author

I'll merge this and open up a follow-up PR that fixes unadvertise for both cases (and add a relevant unit test).

@EzraBrooks EzraBrooks merged commit f8d6dde into develop Mar 19, 2024
6 checks passed
@EzraBrooks EzraBrooks deleted the add-async-form-to-service-advertisement branch March 19, 2024 19:50
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.

3 participants