Skip to content
This repository has been archived by the owner on Apr 3, 2023. It is now read-only.

feat(connector): add discord connector (logto-io/logto#1876) #11

Merged
merged 4 commits into from
Sep 16, 2022
Merged

feat(connector): add discord connector (logto-io/logto#1876) #11

merged 4 commits into from
Sep 16, 2022

Conversation

FlurryNight
Copy link
Contributor

@FlurryNight FlurryNight commented Sep 8, 2022

Summary

Add Discord connector (logto-io/logto#1876)

Testing

UT and Locally Tested

@FlurryNight
Copy link
Contributor Author

@darcyYe ,thanks for re-approving the workflows.

Should i add an eslint disable complexity comment, keeps trowing that because of the discord avatar url xD, if i remove it it passes locally, can u look into it? Is there a better way i can do that. Discord doesn't pass an url for the avatar..

Thanks again

@darcyYe
Copy link
Contributor

darcyYe commented Sep 13, 2022

@darcyYe ,thanks for re-approving the workflows.

Should i add an eslint disable complexity comment, keeps trowing that because of the discord avatar url xD, if i remove it it passes locally, can u look into it? Is there a better way i can do that. Discord doesn't pass an url for the avatar..

Thanks again

Hi @FlurryNight, you may disable the complexity lint rule to make it pass the check as it should not be a blocker.

packages/connector-discord/src/index.ts Outdated Show resolved Hide resolved
packages/connector-discord/src/index.ts Outdated Show resolved Hide resolved
packages/connector-discord/src/index.ts Outdated Show resolved Hide resolved
@FlurryNight
Copy link
Contributor Author

@darcyYe , Done the suggested changes, Lets see if this suppresses the need for the /* eslint-disable complexity */

@FlurryNight
Copy link
Contributor Author

I think i've removed smth by mistake , im on mobile just received the failed notification , let me see

@FlurryNight
Copy link
Contributor Author

Should be building 🛠 fine now, i did those changes on mobile, Dcoder editor must have passed out 😂

@darcyYe
Copy link
Contributor

darcyYe commented Sep 14, 2022

No worries!
Let me try the Discord connector's functionality (I could not test it yesterday due to some minor bugs in our core).
The code looks good 😄

@FlurryNight
Copy link
Contributor Author

No worries!
Let me try the Discord connector's functionality (I could not test it yesterday due to some minor bugs in our core).
The code looks good 😄

Okay, however eslint is not happy yet, want me to add the disable rule comment?

@darcyYe
Copy link
Contributor

darcyYe commented Sep 14, 2022

Hi @FlurryNight, I've tried the connector and it worked smoothly!
You may disable the complexity lint rule, or I can do that for you. I'll double-check the version of the dependencies and some other details before approving, cheers!

packages/connector-discord/src/constant.ts Outdated Show resolved Hide resolved
packages/connector-discord/src/index.ts Show resolved Hide resolved
packages/connector-discord/src/index.ts Outdated Show resolved Hide resolved
Copy link
Member

@charIeszhao charIeszhao left a comment

Choose a reason for hiding this comment

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

Looks good to me

@FlurryNight
Copy link
Contributor Author

Damn, forgot to update the getUserInfo UT, let me see if i can do that quick on my phone should be just adding verified field

@FlurryNight
Copy link
Contributor Author

@charIeszhao ,changed , can u verify it for me?

Thanks

@darcyYe
Copy link
Contributor

darcyYe commented Sep 15, 2022

no worries, after an offline discussion with @charIeszhao, we may keep both email and verified for future use. Would you mind me making the change to your code? @FlurryNight

@FlurryNight
Copy link
Contributor Author

no worries, after an offline discussion with @charIeszhao, we may keep both email and verified for future use. Would you mind me making the change to your code? @FlurryNight

Okay, go ahead!

@darcyYe
Copy link
Contributor

darcyYe commented Sep 15, 2022

@FlurryNight can we temporarily hold this PR for a few days until early next week?
The change I intended to make depends on the @logto/connector-core package, which shares the same version as @logto/core repo does. We need to update @logto/connector-core before making changes to your code. We may find a temporary solution if you want to merge this PR ASAP. Let me know what you would like 😃

@FlurryNight
Copy link
Contributor Author

@FlurryNight can we temporarily hold this PR for a few days until early next week?
The change I intended to make depends on the @logto/connector-core package, which shares the same version as @logto/core repo does. We need to update @logto/connector-core before making changes to your code. We may find a temporary solution if you want to merge this PR ASAP. Let me know what you would like 😃

Hi, Alright.
I agree with the way you think is best.

@darcyYe
Copy link
Contributor

darcyYe commented Sep 16, 2022

@FlurryNight , we have @logto/connector-core published and made the change to expose email_verified as UserInfoResponse. I'll have this PR merged, and Logto Team will release a new version of @logto/connectors this weekend, cheers!
Great thanks for building the Discord connector!

@darcyYe darcyYe merged commit 50acc93 into logto-io:master Sep 16, 2022
@FlurryNight
Copy link
Contributor Author

@FlurryNight , we have @logto/connector-core published and made the change to expose email_verified as UserInfoResponse. I'll have this PR merged, and Logto Team will release a new version of @logto/connectors this weekend, cheers!
Great thanks for building the Discord connector!

Amazing, you're welcome

Cheers!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants