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 async/await support #332

Closed
aronbudinszky opened this issue Sep 24, 2022 · 11 comments · Fixed by #427
Closed

Add async/await support #332

aronbudinszky opened this issue Sep 24, 2022 · 11 comments · Fixed by #427
Labels
enhancement New feature or request

Comments

@aronbudinszky
Copy link
Contributor

aronbudinszky commented Sep 24, 2022

Description
Since server-side Swift projects like Vapor now support async/await APIs, it would nice if meilisearch-swift also added this.

Basic example
As opposed to the completion parameter style currently used, an async/await call would look like so:

let result = await index.updateDocuments(documents: [/** data **/], primaryKey: "id")
switch result {
case .success:
    // ....
case .failure(let error):
    // ....
}

Proposed solution
I have already created extensions in my local project to support some of the methods that I use. For example updateDocuments() is covered by this implementation:

public func updateDocuments<T>(
  documents: [T],
  encoder: JSONEncoder? = nil,
  primaryKey: String? = nil) async -> Result<TaskInfo, Swift.Error> where T: Encodable {
      return await withCheckedContinuation { continuation in
          self.updateDocuments(documents: documents, encoder: encoder, primaryKey: primaryKey) { res in
              continuation.resume(returning: res)
          }
      }
}

Proposal is to add these bridging methods for all methods on Indexes in perhaps an Indexes+async.swift extension. Additionally unit tests will need to be added to cover async calls.

@aronbudinszky
Copy link
Contributor Author

I am happy to implement this, but before I do so wanted to get some input on (a) whether this feature has as much merit as I think it does (b) whether the proposed solution is acceptable.

Note that if implemented all new and updated methods on Indexes will need to be mirrored to its async counterpart which adds a bit of overhead to maintenance.

@brunoocasali
Copy link
Member

Hi @aronbudinszky!

I'm not a Vapor expert, but I can see good potential with this feature you're proposing. I think it could be nice to adopt.
Also, I suggest you start very small for one or two methods, and then we can iterate in the code reviews. This way, you don't invest that much time upfront, and it will be easier for me to review :)

@brunoocasali
Copy link
Member

The integrations team does not have the resources to implement or review that. If someone wants that and could ensure through tests, it does work, we can merge that.

Until then, I'm going to close this to clean the repository.

@brunoocasali brunoocasali closed this as not planned Won't fix, can't repro, duplicate, stale Aug 15, 2023
@Sherlouk
Copy link
Collaborator

Hi @brunoocasali, appreciate your last comment. New to the project 👋

Async/Await continues to grow in it's usage across the Swift ecosystem, be it with server-side Swift (and Vapor as raised above) but even just in iOS itself and other Apple platforms. SwiftUI for example continues to implement more and more async functionality built in.

I've actually just spent an hour throwing together a Vapor service for this library, and it's quickly obvious how the lack of async/await functions makes it more difficult to utilise this with a modern codebase.

The use of "withCheckedContinuation" is a common pattern within libraries which want to maintain backwards support but still have a view for the future. Apple themselves utilise this in their open-source packages such as swift-nio. I have no doubt that the solution mentioned in the thread would absolutely work.

I'm going to open a PR tomorrow which demonstrates this for you on a single API call. I'd love to see this moved forward.

@aronbudinszky
Copy link
Contributor Author

I for one am happy to contribute. Haven't had the time to add this so far. But as @Sherlouk mentioned - it is more and more important.

Sherlouk added a commit to Sherlouk/meilisearch-swift that referenced this issue Sep 16, 2023
This acts as an example for the conversation being held in issue meilisearch#332.

Async/await continues to grow in popularity and all modern codebases are moving towards its usage. This commit demonstrates how we can add support for Swift Concurrency without breaking existing compatibility with older operating systems.

For a test I have taken an existing test, copied it, and converted it for async/await. In this instance, removing the XCTestExpectation and simply using an async test available since Xcode 13 back in September 2021.
@Sherlouk
Copy link
Collaborator

Great to hear from you @aronbudinszky! By no means wanting to 'steal' this from you, but I have opened a PR (#410) to demonstrate your idea.

If time is difficult for you, and with the blessing from Bruno, I'm happy to flesh the PR out to cover all currently supported APIs. Async/await is really important for my use cases 😄

@curquiza
Copy link
Member

Reopening since it looks like community has taken it again 😄

@curquiza curquiza reopened this Sep 21, 2023
@Sherlouk
Copy link
Collaborator

Thank you for re-opening Clem.

@aronbudinszky, as you voiced an opinion, would you mind reviewing PR #410? It would be nice to see what another user of the package thinks.

While this would introduce a lot of extra functions, and definitely increase the maintenance overhead, I think the value of such a feature to users (especially in modern applications) makes this necessary.

I'd be happy to scale up and support the maintenance of it if we're in agreement.

There's a potential solution too in the future if we upgrade the Swift version to v5.9 we could use macros to automatically generate the async/await versions. I think this is too big a leap in support to action right now (it was only released this week) but something to consider down the line.

@curquiza
Copy link
Member

curquiza commented Sep 28, 2023

@aronbudinszky, as you voiced an opinion, would you mind reviewing PR #410? It would be nice to see what another user of the package thinks.

Would love to have the review of the community on this. I'm not a swift developer (we don't have any at Meili), but I'm the only one who has the time to take care of the minimum maintenance of this repo. So, I can bring the Meilisearch knowledge you need 😄

@aronbudinszky
Copy link
Contributor Author

I added a review just now, apologies for the delay. There's only one minor request but other than that the solution is perfect.

@aronbudinszky
Copy link
Contributor Author

aronbudinszky commented Sep 28, 2023

...and thanks @Sherlouk for pushing this forward!

I'm happy to review any further changes and in a week or two I should have some time to assist in adding async support to other functions.

Sherlouk added a commit to Sherlouk/meilisearch-swift that referenced this issue Sep 28, 2023
This acts as an example for the conversation being held in issue meilisearch#332.

Async/await continues to grow in popularity and all modern codebases are moving towards its usage. This commit demonstrates how we can add support for Swift Concurrency without breaking existing compatibility with older operating systems.

For a test I have taken an existing test, copied it, and converted it for async/await. In this instance, removing the XCTestExpectation and simply using an async test available since Xcode 13 back in September 2021.
meili-bors bot added a commit that referenced this issue Sep 28, 2023
410: Add async overload for search API r=curquiza a=Sherlouk

# Pull Request

⚠️ I've marked this as draft as it current works to provide an example of how we could add async/await support to the library. I am more than happy to expand this to **all** currently supported APIs.

## Related issue

Relates to #332 (doesn't close as only adds support for one API)

## What does this PR do?
- Adds an async/await override to an existing search API providing users with more choice.

## PR checklist
Please check if your PR fulfills the following requirements:
- [x] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
- [x] Have you read the contributing guidelines?
- [x] Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!


Co-authored-by: James Sherlock <[email protected]>
@meili-bors meili-bors bot closed this as completed in eac972b Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants