Skip to content

Conversation

@jankapunkt
Copy link
Member

@jankapunkt jankapunkt commented May 26, 2023

  • use @node-oauth/oauth2-server
  • update all dependencies to latest
  • add code coverage to tests
  • add GitHub actions CI
  • replace jshint with eslint

Test this with your local setup by cloning this repo and checkout the branch 3.0.0

Other notes:

  • we don't move to ES6 in this PR, because that would make the PR way too big!
  • there are no functional changes in the code so it should be non-breaking (although the transition to @node-oauth/oauth2-server would be breaking if you used oauthjs before)
Copy link

@shrihari-prakash shrihari-prakash left a comment

Choose a reason for hiding this comment

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

The changes look good to me, and from a functionality standpoint as well, I can confirm that things seem to be working fine. I've tested the following grant types from my framework and no problems with this:

  1. Code Response Type
  2. Authorization Code
  3. Password
  4. Client Credentials
@shrihari-prakash
Copy link

Might also be a good idea to remove bluebird dependency since promises are native in a good amount of node versions. But I don't know if there is already a plan for this.

@HappyZombies
Copy link
Member

HappyZombies commented May 26, 2023

Might also be a good idea to remove bluebird dependency since promises are native in a good amount of node versions. But I don't know if there is already a plan for this.

@shrihari-prakash
I agree but let's keep that for another version/MR, this whole module should/will probably be refactored entirely at some point. The focus on this MR is to just update the node-oauth dependency and other deps.

Copy link
Member

@HappyZombies HappyZombies left a comment

Choose a reason for hiding this comment

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

Code Review Completed

@jankapunkt
Copy link
Member Author

Thanks @HappyZombies @shrihari-prakash I will merge and then create a release under our org

@jankapunkt jankapunkt merged commit c2a8b11 into master May 26, 2023
@jankapunkt jankapunkt mentioned this pull request May 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants