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

Async javascript #26

Closed
wants to merge 4 commits into from
Closed

Async javascript #26

wants to merge 4 commits into from

Conversation

joemasilotti
Copy link
Member

This PR converts all JavaScript calls to async functions. It cleans up out test suite but I'm not sure I like the idea of every developer interaction with Strada needing an await call.

@olivaresf
Copy link
Member

olivaresf commented Dec 13, 2023

It cleans up out test suite but I'm not sure I like the idea of every developer interaction with Strada needing an await call.

While I understand where you're coming from, Swift is moving towards async await as the default, so I don't see that as a problem. In addition, I think the intent is much clearer now that you've incorporated async. I've tested this branch and I noticed the following changes:

Closures

@objc func performAction(_ sender: Any) {
    actionButton = sender as? ActionButton
    reply(to: Event.connect.rawValue)
}

Async Await

@objc func performAction(_ sender: Any) {
    actionButton = sender as? ActionButton
    Task {
        await reply(to: Event.connect.rawValue)
    }
}

I needed to add a Task now when interacting with Strada, but it's also much clearer that this is not a synchronous call. I like that.

@joemasilotti
Copy link
Member Author

That is the only public API change it seems. And wrapping something in a task definitely doesn't add a ton of overhead for a developer.

While we are messing with these functions I'm wondering if the error handling could also be improved. I don't think we actually need to wrap things in the JavaScriptResult struct and instead could return Any? and have everything throw.

What are your thoughts on that? It would push all the way to the public API:

@objc func performAction(_ sender: Any) {
    actionButton = sender as? ActionButton
    Task { try await reply(to: Event.connect.rawValue) }
}

The response is already discardable via @discardableResult. Does it make sense to expose that an error can be thrown, too?

@joemasilotti
Copy link
Member Author

Closed in favor of #29.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants