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

Remove lodash dependency #37

Merged
merged 3 commits into from
Oct 14, 2021
Merged

Remove lodash dependency #37

merged 3 commits into from
Oct 14, 2021

Conversation

jorenvandeweyer
Copy link
Member

Resolves issue #23

@HappyZombies
Copy link
Member

Awesome, we will take a look at this soon after #30 is merged into development. But so far looks good, excited for this removal.

@HappyZombies HappyZombies added dependencies 🔌 Pull requests that update a dependency file on hold 🛑 We will look into it at a later time labels Oct 13, 2021
@jwerre
Copy link
Contributor

jwerre commented Oct 13, 2021

Hey! I was just getting ready to do this. Thanks @jorenvandeweyer I'll check it out now.

@HappyZombies HappyZombies self-requested a review October 13, 2021 14:33
jwerre
jwerre reviewed Oct 13, 2021
View reviewed changes
lib/errors/oauth-error.js Outdated Show resolved Hide resolved
@jankapunkt jankapunkt linked an issue Oct 13, 2021 that may be closed by this pull request
Remove lodash dependency #23
Closed
jankapunkt
jankapunkt reviewed Oct 13, 2021
View reviewed changes
Copy link
Member

@jankapunkt jankapunkt 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, I have only the one thing to discuss but won't block due to it. I'd rather want to hear what you think. We can also create a separate issue on this one.

lib/handlers/authorize-handler.js Outdated Show resolved Hide resolved
@jankapunkt
Copy link
Member

@jorenvandeweyer can you please pull development? We merged a few PRs lately

jankapunkt
jankapunkt approved these changes Oct 14, 2021
View reviewed changes
Copy link
Member

@jankapunkt jankapunkt 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. I would squash merge this into one commit and name it using our commit conventions. If no one has any arguments against I would proceed.

@HappyZombies
Copy link
Member

HappyZombies commented Oct 14, 2021 •
edited
Loading

@jankapunkt I haven't had much time to look at myself but I trust you, you are good to merge!

@HappyZombies HappyZombies removed the on hold 🛑 We will look into it at a later time label Oct 14, 2021
HappyZombies
HappyZombies approved these changes Oct 14, 2021
View reviewed changes
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.

Excellent!

@HappyZombies HappyZombies merged commit 8e05cb5 into node-oauth:development Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jwerre jwerre jwerre approved these changes

@HappyZombies HappyZombies HappyZombies approved these changes

@jankapunkt jankapunkt jankapunkt approved these changes

Assignees
No one assigned
Labels
dependencies 🔌 Pull requests that update a dependency file
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove lodash dependency
4 participants
@jorenvandeweyer @HappyZombies @jwerre @jankapunkt