async/await #94
async/await #94
Conversation
|
|
||
| /// The route that the OAuth provider calls when the user has been authenticated. | ||
| /// | ||
| /// - Parameter request: The request from the OAuth provider. | ||
| /// - Returns: A response that should redirect the user back to the app. | ||
| /// - Throws: An errors that occur in the implementation code. | ||
| func callback(_ request: Request) throws -> EventLoopFuture<Response> | ||
| func callback(_ request: Request) async throws -> Response |
There was a problem hiding this comment.
We should consider the possible race conditions that this could cause. To mitigate the possibility of a race condition, we may add the @Sendable to the function definition.
Could you help us add that to the code?
There was a problem hiding this comment.
@Sendable should not be added to function declarations - it's a half broken implementation that currently just disables sendable checking inside that function. We should definitely enable Sendable checking on the library though
| public var scope: [String] = [] | ||
| public var callbackURL: String | ||
| public let accessTokenURL: String = "https://www.deviantart.com/oauth2/token" | ||
|
|
||
| public required init(callback: String, completion: @escaping (Request, String)throws -> (Future<ResponseEncodable>)) throws { | ||
| public required init(callback: String, completion: @escaping (Request, String)async throws -> Response) throws { |
There was a problem hiding this comment.
Could we add the space between the async and the Request, String)?
| router.get(callbackURL.pathComponents, use: callback) | ||
| router.get(authURL.pathComponents) { req -> EventLoopFuture<Response> in | ||
| public func configureRoutes(withAuthURL authURL: String, authenticateCallback: ((Request) async throws -> Void)?, on router: RoutesBuilder) async throws { | ||
| router.get(callbackURL.pathComponents, use: self.callback) |
There was a problem hiding this comment.
Should we keep the self. if we are not in the init or closure?
| @@ -16,7 +15,7 @@ public class GitHubRouter: FederatedServiceRouter { | |||
| return headers | |||
| }() | |||
|
|
|||
| public required init(callback: String, completion: @escaping (Request, String) throws -> (EventLoopFuture<ResponseEncodable>)) throws { | |||
| public required init(callback: String, completion: @escaping (Vapor.Request, String) async throws -> Vapor.Response) async throws { | |||
There was a problem hiding this comment.
I guess we don't need to add the Vapor. declaration, Should we keep it?
|
any update? |
|
Just an FYI I'm going to be doing an Imperial code sprint next week to go through all the issues and PRs |
|
i did a small rewrite, in which i applied the pattern of the jwt-kit and layered oauth on top of it. |
|
Closing in favour of #109 |
Not perfect but a starting point. Also definitely not tested for all providers.
I need this for Twitter/X and so I thought I'll bite the bullet and make it async/await.