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

bcrypt is an optional dependency and allow anonymous login #13

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sokra
Copy link

@sokra sokra commented Sep 11, 2013

two little changes I needed to use this great lib.

vesse added a commit to vesse/node-ldapauth-fork that referenced this pull request Sep 18, 2013
Cherry-picked a commit by @sokra from trentm#13. This
probably fixes trentm#9 as well when cache is not needed.
@cstephe
Copy link

cstephe commented Dec 10, 2013

not sure about the bcrypt, but the anonymous login fix would be very helpful to me. I can submit as a separate change set... @vesse Is there a reason not to remove that check?

@sokra
Copy link
Author

sokra commented Dec 10, 2013

@vesse picked the commit for another repo which has already many fixes including the anonymous login. You can switch from node-ldapauth to node-ldapauth-fork... I've done so...

@cstephe
Copy link

cstephe commented Dec 10, 2013

fantastic, keep up the good work!

@stryju
Copy link

stryju commented Jul 17, 2014

not really sure that you are using optionalDependencies correctly...

If a dependency can be used, but you would like npm to proceed if it cannot be found or fails to install, then you may put it in the optionalDependencies hash. This is a map of package name to version or url, just like the dependencies hash. The difference is that failure is tolerated.

It is still your program's responsibility to handle the lack of the dependency.

source


so, you would have to check if bcrypt is present (probably with try { ... } catch() { ... })

@sokra
Copy link
Author

sokra commented Jul 17, 2014

ldapauth only requires bcrypt if you pass cache: true. So if you don't use the caching feature you can omit it. If you use the cache and bcrypt isn't loaded it throws an exception...

@trentm
Copy link
Owner

trentm commented Jul 17, 2014

@vesse Just saw all your great work on node-ldapauth-fork. What would you think about merging your changes into this repo, and me giving you commit and publish rights for this repo and its npm module?

@vesse
Copy link
Contributor

vesse commented Jul 18, 2014

@trentm Thanks, that would be great!

@solomongifford
Copy link

Whats the status on the merge?

vesse added a commit to vesse/node-ldapauth-fork that referenced this pull request Feb 19, 2015
Cherry-picked a commit by @sokra from trentm#13. This
probably fixes trentm#9 as well when cache is not needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants