-
Notifications
You must be signed in to change notification settings - Fork 67
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
Feature/async await #71
Conversation
@@ -36,7 +36,7 @@ jobs: | |||
- focal | |||
- bionic | |||
tag: | |||
- swift:5.4 | |||
- swift:5.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you are forcing everyone to upgrade to swift 5.5? Most other initial implementations of async/await in existing libraries have been fairly slim additions on top of what already existed. And those additions hidden behind a #if compiler(>=5.5) && canImport(_Concurrency)
thus removing the swift 5.5 requirement for the whole package. Another common practice has been to move all the new concurrency code into its own folder so it can easily be found.
Checkout the implementations from Vapor, gRPC and my own Soto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, @adam-fowler. I just did that because I was having a hard time making things work and that was one of my attempts, 🙃. I saw that that #if compiler(>=5.5) && canImport(_Concurrency)
check is the way to go. I'm actually using it already. Moving all the concurrency code into a separate folder sounds like a good idea. Another thing that I'm a bit confused is that Xcode keeps yelling me about @available(macOS 12, *)
and, for now, I'm adding those all over the place so I can keep focusing on the work itself. Those are mostly sprinkled all over the tests. Are those really necessary?
My plan is to not make a breaking change with this async-await support. However, I want all the tests to be using this new API. I'm not sure it's worth to duplicate all tests, one for each version of the API. That's why I updated the CI requirement to Swift 5.5. I could add a new build variant with Swift 5.4 to the CI configuration, just to be sure we're building. I'm not sure there will be any tests left which don't require Swift 5.5, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you move all your code to a separate folder you can add the @available check to the extension of each type instead of every function.
Yeah the question of testing is a little awkward. For SotoCore I ended up leaving most of my tests using the EventLoopFuture apis. And just added a single test for each async function. That worked fine for me but then SotoCore async code had very few additional functions. Again I did this in a separate file.
One thing to also note, the test discovery on Linux doesn't deal with async functions. This will be fixed but in the meantime I ended up added a helper function that kicked off a Task and used DispatchGroup to wait until that Task was complete.
@available(macOS 12.0, iOS 15.0, watchOS 8.0, tvOS 15.0, *)
public func XCTRunAsyncAndBlock(_ closure: @escaping () async throws -> Void) {
let dg = DispatchGroup()
dg.enter()
Task {
do {
try await closure()
} catch {
XCTFail("\(error)")
}
dg.leave()
}
dg.wait()
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey! I took a look at your code and looks nice! I found a point that looked a little confusing about a constructor to use description, so commented there.
I am looking forwarding for testing this =)
let name: String | ||
var description: String? = nil | ||
|
||
init(name: String) { | ||
self.name = name | ||
} | ||
|
||
public required init(stringLiteral string: StringLiteralType) { | ||
self.name = "" | ||
self.description = string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are those the description that end up becoming GraphQL documentation? Could you add an example in the docs on how to use this one?
Also, why is the name
property empty here? Can't we initialize a field with the required name and a description?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found out about the description
modifier. Never mind this one.
Really appreciate you working on this feature, I'm super eager to switch to using Async/Await. You have any sense of a possible timeline for getting this merged? Thanks again! |
Async/await support by @NeedleInAJayStack just merged, a new release is coming soon! |
Is this pull request still necessary? My understanding is async await support was added in #78. |
I've left it open for the moment because it includes more than just |
Oh got it - that makes sense! |
No description provided.