Move to std::future::Future about yup-oauth2 HOT 5 CLOSED

Okay. I have a working change here:

ggriffiniii@e1dca50

it pins to the alpha versions of tokio and hyper which is why I didn't just send it as a pull request. Figuring you probably want to wait for those pieces to release.

There are still a lot of remaining things that could be cleaned up. I think there's an unnecessary amount of cloning and things that was probably to workaround lifetime issues with the callbacks. Some of that may involve changing function signatures to accept slightly different types. I'm thinking now would be a good time to try and do all of these things since moving to std futures is already a breaking change.

Here are some of the breaking changes I would propose after looking over the code while making this change.

• I would change InstalledFlowMethod::HTTPRedirect to not take a port number instead just do what HTTPRedirectEphemeral does. I can't think of any reason a user would want to specify a port number.
• Force all delegates to implement Send + Sync and no longer require them to implement Clone.
• Change delegate methods to take &self rather than &mut self.
• In general minimize allocations by changing methods to accept slices rather than String and Vec when all they want is to read them.

Do you have a problem with any of the above ideas? I can throw together some of the changes to see how they turn out. If they don't look like an improvement I have no problem abandoning them.

from yup-oauth2.

Yes, I would be very happy about a PR! I have become a bit tired of the constant change so I was happy with the state as it was, but now that the ecosystem becomes more stable this would be very nice to have.

from yup-oauth2.

Okay. I've been looking into this. I was hoping it would be a mostly mechanical change, but it is definitely not. A lot of the futures utilities the current implementation uses are no longer available because async/await is a more natural way to solve it, but that means restructuring the code away from the callbacks that exist today. Overall, it should definitely be an improvement to the code quality, but just beware that it's going to be a massive change without any ability to break it into a series of smaller commits.

Additionally, some of the mutexes in the current code are being held while futures are executing. This is asking for trouble and could easily lead to system wide deadlocks. Refactoring the locking to correct that is most likely going to require changing some of the trait definitions. Since this is going to be a major breaking change anyway I'm hoping that's ok.

from yup-oauth2.

w.r.t. the points

1. I agree, the current way is for backwards compatibility but it shouldn't be a big issue to break it in a major release
2. yes
3. if possible, yes
4. if possible, yes -- I unfortunately added Clone or non-ref arguments when refactoring for tokio. I didn't have a whole lot of asynchronous rust experience at the time, and despite spending quite some time I didn't manage to figure out something better (and often the compiler errors did sound reasonable)

so, no, I don't have problems with your proposed changes!

from yup-oauth2.

This was just merged

from yup-oauth2.