potential issue with optionalDependency @ bcrypt

[stryju]

as explained @ trentm#13 (comment)

you should wrap the bcrypt calls with try { ... } catch( err ) { ... } or move the bcrypt back to normal dependencies...

[vesse]

Sorry, but I'm not planning to do that. If it were a normal dependency Windows people would have hard time installing the package unless they know they need to use some weird flags. If I wrap it in try/catch and a Windows user tries to use cache they would not see a nice big crash but maybe an error message given they are using log4js. This does not really apply to any else scenario, at least that I know of.

[stryju]

If I wrap it in try/catch and a Windows user tries to use cache they would not see a nice big crash but maybe an error message given they are using log4js.

hm... wouldn't handling it on crash and "crashing" with a good explanation why that could've happened be a desired solution?

[vesse]

That would be an option as well. It would be needed in the constructor only so the tiny overhead of try-catch would mean nothing. OK, maybe I could do that :)

[stryju]

there you go! 👍

i think that's a nicer solution than the current one :-)

[stryju]

simple try ... catch in https://github.com/vesse/node-ldapauth-fork/blob/master/lib/ldapauth.js#L104-L107 would suffice, right?

also, i think it would make more sense to have a reference to bcrypt in "main" scope of ldapauth script (so no subsequent requires are done) - or should that have a different issue in the tracker?

[vesse]

Yes (did it already). It still throws but now with a longish description. Thanks. Maybe I don't get bcrypt related bug reports anymore :)

[stryju]

🍻 👍