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

feat(spotify-auth): add Spotify OAuth plugin #2989

Merged
merged 1 commit into from
Apr 19, 2019

Conversation

lutangar
Copy link
Contributor

@lutangar lutangar commented Apr 9, 2019

I'm not the maintainer of the cordova-spotify-oauth plugin.
So we should probably wait for @NeoLegends review/approval on this before merging anything.

@NeoLegends
Copy link

NeoLegends commented Apr 9, 2019

Could you explain in 1-2 sentences what this does? Does it expose the cordova plugin as ionic plugin?

If it's that, I think it might be a good idea to reexport the typings from our code instead of re-declaring them here to reduce duplication, but other than that, LGTM.

@lutangar
Copy link
Contributor Author

lutangar commented Apr 9, 2019

Does it expose the cordova plugin as ionic plugin?

Yes, this is what it does. It describes the plugin as an injectable service/object for angular.

I think it might be a good idea to reexport the typings from our code instead of re-declaring them here to reduce duplication

Yeah I would love to do that, I don't know if's possible though. The explanations are not very thorough, and there is nothing on this specific matter.

@NeoLegends
Copy link

So normally you'd just add the project as dependency to the plugin and export the types using ESM syntax.

plugin: 'cordova-spotify-oauth',
pluginRef: 'cordova.plugins.spotifyAuth',
repo: 'https://github.com/Festify/cordova-spotify-oauth',
install: 'ionic cordova plugin add resgrid-cordova-plugins-rollbar --variable LOCAL_STORAGE_KEY="SpotifyOAuthData"',

Choose a reason for hiding this comment

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

What's resgrid-cordova-plugins-rollbar? Naively I'd say you'd want to install cordova-spotify-oauth here instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed 😅

@lutangar
Copy link
Contributor Author

lutangar commented Apr 9, 2019

So normally you'd just add the project as dependency to the plugin and export the types using ESM syntax.

Yeah definitely, but in the case of this project (ionic-native), the dependencies of a plugin seems to be handled via the @Plugin decorator... the duplicated declarations seems like... the way this collection of plugins are working...

Surely we could generate and push these declarations from the cordova-spotify-oauth repo, but it would require a bit of automation.

@danielsogl danielsogl self-assigned this Apr 9, 2019
@danielsogl danielsogl added the target: minor This PR is targeted for the next minor release label Apr 9, 2019
@NeoLegends
Copy link

Oh well. Okay, then, I guess you can merge this now.

@lutangar
Copy link
Contributor Author

Ping @danielsogl

@danielsogl danielsogl merged commit 21dc1f9 into danielsogl:master Apr 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants