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

Removed Bluebird and Promisify-Any #107

Closed
wants to merge 12 commits into from
Closed

Removed Bluebird and Promisify-Any #107

wants to merge 12 commits into from

Conversation

jwerre
Copy link
Contributor

@jwerre jwerre commented Dec 13, 2021

Summary

Removed support for callbacks and removed bluebird and promisify-any dependencies. I'm not sure how you want to merge this since this is a major version update (v5).

Linked issue(s)

Involved parts of the project

Added tests?

No tests added but many are refactored and changed.

OAuth2 standard

Reproduction

@jwerre
Copy link
Contributor Author

jwerre commented Dec 13, 2021

@jankapunkt CodeQL is picking up a hardcoded password in the test suite. Is there a way to ignore tests? What is the best solution for this?

@jorenvandeweyer
Copy link
Member

You can mark it as "used in test". Or make a variable for the authorization header.

@jankapunkt
Copy link
Member

I always use Math.random().to string(16) to generate a simple random hex password if the password itself is not to be tested.

@jankapunkt
Copy link
Member

@jwerre this is a great step towards modern JS but I'd like to delay it until we merged the open PRs that fix code and/or compliance issues otherwise we will have to resolve lots of merge conflicts what do you think?

@jwerre
Copy link
Contributor Author

jwerre commented Dec 14, 2021

@jankapunkt Yes, I was watching all the PRs coming in and thinking the same thing. No rush on this.

@Uzlopak
Copy link
Collaborator

Uzlopak commented Dec 18, 2021

If we merge #109 then this whole branch will be unmergeable.

@jankapunkt
Copy link
Member

@Uzlopak partially yes but it already needs refactoring/merging work so I think we should just set a cut after the recent 5 or 6 open PRs and exclusively focus on this one all together then it will be fixed fast and we can continue

@jankapunkt jankapunkt added code quality 🧽 Relating to code quality backwards breaking ✂️ This change will not work with the current version of the module. dependencies 🔌 Pull requests that update a dependency file labels Jan 7, 2022
@jankapunkt jankapunkt added this to the v5 milestone Mar 29, 2022
@esatterwhite
Copy link

@jankapunkt Is this still valid? can it be merged?

@jankapunkt
Copy link
Member

@esatterwhite this is still to be done, however there are a few things blocking:

I'm currently allocating more time but support from the community is also highly appreciated. Please watch the progress in #70, once merged we will definitely continue here.

@jankapunkt
Copy link
Member

Hey @jwerre I struggled hard to merge this and by the time I realized that the merge lost some code from development, the two branches are too far away from each other, also, because #109 changed many ES5 classes to ES6 classes and the diff was unbearable to comprehend...

I used your impl. as blueprint for creating #190 which I started from scratch, based on the latest development state.

I will close this as superseded by #190 please don't feel bad about it, the merge was too risky to loose recent fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards breaking ✂️ This change will not work with the current version of the module. code quality 🧽 Relating to code quality dependencies 🔌 Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants