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

Support ASWebAuthenticationSession #335

Merged

Conversation

Kyome22
Copy link
Contributor

@Kyome22 Kyome22 commented Dec 3, 2020

I implemented to solve this issue #334
I also updated the SwifterDemoMac.

@meteochu
Copy link
Contributor

meteochu commented Dec 3, 2020

Hi there, thanks for the PR. I just quickly looked at it and there seems like quite a bit of code duplication and I wonder if there's a cleaner way to support these authentication methods across different platforms.

@Kyome22
Copy link
Contributor Author

Kyome22 commented Dec 3, 2020

@meteochu
Thank you for looking at my request. I think so too.
There is much code duplication. However, because of the complexity of the conditional branching by platform and their OS versions, it is difficult for me to determine the extent to be solved at once.

By the way, do you think we should support ASWebAuthenticationSession to iOS with this pull request? I think we should do it as soon as possible according to apple's references.

@meteochu
Copy link
Contributor

meteochu commented Dec 3, 2020

I've actually not looked at ASWebAuthenticationSession in the past. I just gave it a quick glance and it does seem quite promising. That said, I think this could be part of a big clean-up in the project that potentially supports Twitter API v2, as well as making this the default authentication to remove friction with the callbacks and to better support SwiftUI.

I don't really have the time just yet to look at all of it but am happy to explore at some point.

- Removed code duplication as much as possible.
- Improved Demo App and made the structure of the iOS demo and the macOS demo similar.
@Kyome22 Kyome22 changed the title Support ASWebAuthenticationSession for macOS Support ASWebAuthenticationSession Dec 7, 2020
@Kyome22
Copy link
Contributor Author

Kyome22 commented Dec 7, 2020

@meteochu
I've improved it to support iOS.
I checked its operation used the keys of my Twitter client app. The program seems to work properly on both iOS and macOS. I tried to reduce code duplication. How about that? I want you to merge if you like this.

SwifterDemoMac/ViewController.swift Show resolved Hide resolved
SwifterDemoiOS/AuthViewController.swift Outdated Show resolved Hide resolved
Sources/SwifterAuth.swift Outdated Show resolved Hide resolved
Sources/Swifter.swift Show resolved Hide resolved
Sources/SwifterAuth.swift Outdated Show resolved Hide resolved
Sources/SwifterAuth.swift Outdated Show resolved Hide resolved
@Kyome22
Copy link
Contributor Author

Kyome22 commented Dec 12, 2020

@meteochu Thank you for reviewing. How about my updates?

Swifter.podspec Outdated Show resolved Hide resolved
@meteochu
Copy link
Contributor

Sorry about the delay, got a bit busy wrapping up my academics. The rest of the changes look good to me, once you apply the last change (revert the version change), I'll be happy to merge it :)

@meteochu meteochu merged commit 3c818bb into mattdonnelly:master Dec 23, 2020
@imthath-m imthath-m mentioned this pull request Sep 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants