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

iOS: Removed dependency on legacy Spotify SDK #32

Merged
merged 3 commits into from
Jul 30, 2020

Conversation

manuelsc
Copy link
Contributor

@manuelsc manuelsc commented Jul 9, 2020

Plugin wasn't able to compile for iOS since the Spotify SDK it relies on isn't available anymore. The new sdk appears to have changed its API but I figured there's not need to even pull in that whole sdk dependency since we only need it for the OAuth URL.

I rewrote the code to work without this dependency, it should compile and work with iOS now.

This fixes Issue #28

@tobika
Copy link
Contributor

tobika commented Jul 9, 2020

I'm only using the android auth part of this plugin but there is one advantage of using the sdk for authentication. If you have the Spotify app installed you don't need to enter your credentials and just click on a button to receive your token.

If I'm not mistaken with the web authentication you always have to enter your email/password.

Copy link
Member

@NeoLegends NeoLegends left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing changes! Thank you so much for this. You literally fixed the plugin.

I have some changes addressing the way the URL is constructed. I think there is a more idiomatic and fault-tolerant way to do it instead of manually joining strings.

src/ios/SpotifyOAuthPlugin.swift Outdated Show resolved Hide resolved
src/ios/SpotifyOAuthPlugin.swift Outdated Show resolved Hide resolved
var result = ""
for scope in scopes {
result += scope + " "
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of the for loop I would use the .joined(separator:) method here.

See https://stackoverflow.com/a/24826511

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or rather, when using the idiomatic way of constructing the URL, this method would probably not be required anymore. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I replaced the method with joined as suggested. I'm not sure about your second point so I'll leave this conversation open in case you have further feedback.

@manuelsc
Copy link
Contributor Author

manuelsc commented Jul 9, 2020

I'm only using the android auth part of this plugin but there is one advantage of using the sdk for authentication. If you have the Spotify app installed you don't need to enter your credentials and just click on a button to receive your token.

If I'm not mistaken with the web authentication you always have to enter your email/password.

You are right I was thinking about that as well. But the current implementation only opens the webview on iOS as well.
There appears to be a counterpart to spotifyWebAuthenticationURL namely spotifyAppAuthenticationURL that can be passed to UIApplication to open an installed Spotify app. But I don't know how this URL is constructed, maybe someone with the old spotify sdk installed can give us insight on that?

@NeoLegends
I mostly work on Android which is why I'm not that fit with Swift. I'm sure there's a more idiomatic and cleaner approach. I'll try and look into the changes you suggested.

@NeoLegends
Copy link
Member

@manuelsc Thank you for keeping up! Here is the link to the Documentation for the URL builder https://developer.apple.com/documentation/foundation/urlcomponents

Copy link
Member

@NeoLegends NeoLegends left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leolabs All good, I think. Merge?

@duffdean
Copy link

Is it possible to point my cordova plugins to this pull request? Or do we have to wait until the pull request is merged into the master?

@NeoLegends NeoLegends merged commit 8f95bce into Festify:develop Jul 30, 2020
tobika pushed a commit to lyssna-audiobookplayer/cordova-spotify-oauth that referenced this pull request Nov 2, 2020
* removed spotify sdk dependency and create oauth url by hand

* changed if to guard statement

* cleanup and use urlcomponents to build url

Co-authored-by: Manuel macOS <>
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.

None yet

4 participants