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 take 2 #29

Merged
merged 29 commits into from
Mar 1, 2024
Merged

Async javascript take 2 #29

merged 29 commits into from
Mar 1, 2024

Conversation

joemasilotti
Copy link
Member

@joemasilotti joemasilotti commented Feb 26, 2024

A second take on #26 where the two public functions reply(to:) and reply(with:) are wrapped in a Task {} block. This lets a developer use the async version or non-async version depending on their preference. For example:

// From an async, throwing function:
func doSomething() async throws {
    reply(to: "connect") // Calls async, throwing version.
}

// From a non-async, non-throwing function:
func doSomethingNow() {
    reply(to: "connect") // Calls non-async version.
}

// From a non-async, non-throwing function (again):
func doSomethingLater() {
    Task { try await reply(to: "connect") } // Calls async, throwing version.
}

Notes

  • The tests are currently failing. I think we had some tests that threw errors before that are now being exposed but I haven't dug in, yet.
  • The actual change to the public API is in the most recent commit, 2a9aa0.

@jayohms jayohms requested a review from svara February 26, 2024 18:15
@svara svara self-assigned this Feb 27, 2024
Copy link
Collaborator

@svara svara left a comment

Choose a reason for hiding this comment

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

This looks great @joemasilotti!
I like that we offer both standard and async versions.
I've left some feedback in #30.

Source/BridgeComponent.swift Outdated Show resolved Hide resolved
Source/Bridge.swift Show resolved Hide resolved
@joemasilotti joemasilotti mentioned this pull request Feb 27, 2024
@joemasilotti
Copy link
Member Author

This is ready for re-review @svara!

Copy link
Collaborator

@svara svara left a comment

Choose a reason for hiding this comment

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

Nice work @joemasilotti!
I've noticed we were missing two non async reply(to...) functions. I added them in #31.
By adopting async/await we've made some of the classes not thread safe. I'll fix this in an upcoming PR today.

Source/BridgeComponent.swift Show resolved Hide resolved
Source/BridgeComponent.swift Outdated Show resolved Hide resolved
@joemasilotti
Copy link
Member Author

@jayohms, does Android need an equivalent for this?

If not, I'll merge this then #24 and cut a new release!

@jayohms
Copy link
Collaborator

jayohms commented Feb 28, 2024

@jayohms, does Android need an equivalent for this?

Nope, no relevant concerns on the Android side. Go ahead and merge 👍

@svara
Copy link
Collaborator

svara commented Feb 28, 2024

@joemasilotti Let's wait with the merge. I've run into this issue https://forums.developer.apple.com/forums/thread/701553. I'll open a PR soon ~1h.

@joemasilotti joemasilotti merged commit 0b4b6c0 into main Mar 1, 2024
1 check passed
@joemasilotti joemasilotti deleted the async-javascript-take-2 branch March 1, 2024 17:23
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.

4 participants