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

Add discord socialite provider support #136

Open
wants to merge 1 commit into
base: 4.x
Choose a base branch
from

Conversation

ABSAhmad
Copy link

No description provided.

@ABSAhmad ABSAhmad force-pushed the feature/add-discord-provider branch from e24a6ba to 579d325 Compare April 19, 2024 15:04
@andrewdwallo
Copy link
Owner

Hey. Is this PR completely finished? If so, while you are putting the optional dependency in "suggest" and checking if the class exist before utilizing its functionality, the SocialiteWasCalled event is not part of any dependency this package currently requires (has installed by default).

If this package is going to support other socialite providers, we are going to have to implement more dynamic logic for handling multiple optional social media providers, while avoiding the clutter of if class_exists checks for each provider.

@ABSAhmad
Copy link
Author

Hey. Is this PR completely finished? If so, while you are putting the optional dependency in "suggest" and checking if the class exist before utilizing its functionality, the SocialiteWasCalled event is not part of any dependency this package currently requires (has installed by default).

If this package is going to support other socialite providers, we are going to have to implement more dynamic logic for handling multiple optional social media providers, while avoiding the clutter of if class_exists checks for each provider.

Sorry, somehow missed this reply.

The only way to not have to do class_exists check that I can think of is by not registering Socialite providers in this package but leave it up to the user. Is that fine for you? Or do you have a suggestion on how to do this differently?

@andrewdwallo
Copy link
Owner

Yeah I would instead leave it up to the user but implement something to where the package can support them adding extra socialite providers.

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