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.

Sign up for GitHub

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

Jump to bottom

feat: make alby oauth url customizable #37

Merged
merged 5 commits into from
Jul 26, 2023
Merged

feat: make alby oauth url customizable #37

merged 5 commits into from
Jul 26, 2023

Conversation

pavanjoshi914
Copy link
Contributor

No description provided.

rolznz
rolznz reviewed Jul 26, 2023
View reviewed changes
src/types.ts Outdated Show resolved Hide resolved
rolznz
rolznz reviewed Jul 26, 2023
View reviewed changes
src/OAuth2User.ts Outdated Show resolved Hide resolved
rolznz
rolznz reviewed Jul 26, 2023
View reviewed changes
src/OAuth2User.ts Outdated Show resolved Hide resolved
rolznz
rolznz approved these changes Jul 26, 2023
View reviewed changes
Copy link
Contributor

@rolznz rolznz left a comment

Choose a reason for hiding this comment

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

utACK

@pavanjoshi914
Copy link
Contributor Author

do we need a validation of url here?

@bumi
Copy link
Contributor

bumi commented Jul 26, 2023

do we need a validation of url here?

no it's for developers not end-users. So they are smart and can deal with errors that would happen.
and validation just requires code that needs to be maintained, makes the package more complicated (and any line of code will break at some point, so better avoid it)

@pavanjoshi914
Copy link
Contributor Author

i have tested it with the yarn link things work well!

@bumi
Copy link
Contributor

bumi commented Jul 26, 2023

great. can you add a note of the option to the readme?

@pavanjoshi914
Copy link
Contributor Author

added

rolznz
rolznz reviewed Jul 26, 2023
View reviewed changes
README.md Outdated
@@ -225,6 +225,7 @@ const authClient = new auth.OAuth2User({

const authUrl = authClient.generateAuthURL({
code_challenge_method: "S256",
authorizeUrl: "https://getalby.com/oauth" // endpoint for authorization (replace with the appropriate URL based on the environment)
Copy link
Contributor

Choose a reason for hiding this comment

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

if it's put in the example like this, users will think they need to pass it. Should it be commented out here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

@bumi
Copy link
Contributor

bumi commented Jul 26, 2023

and actually we need two options: the authorize URL and the API URL?

@rolznz
Copy link
Contributor

rolznz commented Jul 26, 2023

and actually we need two options: the authorize URL and the API URL?

@bumi the API url is already configurable (it is called base url)

@bumi bumi merged commit 3b1cdd1 into getAlby:master Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bumi bumi bumi left review comments

@rolznz rolznz rolznz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
🐝 Alby
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pavanjoshi914 @bumi @rolznz