search

sjmeverett / promised-ldap

A thin wrapper over ldapjs to give a promise interface and authentication helpers.
23 stars 11 forks source link

Export filters #1

Open felixfbecker opened 8 years ago

felixfbecker commented 8 years ago

First of all, this module is great. Using vanilla ldap with ES7 async/await was a pain in the ***. I only got one problem, which requires me to include vanilla ldap as dependency too - you don't export the filters (and probably some other things neither like the errors). I think it would also be better to export the client seperatly (because you might add server support) at some point e.g. module.exports = {Client, Server, ...filters}. I also don't quite get why you create your own Client class and not just inherit the the ldap class, but overwrite all methods to return promises. What do you think? I would create a PR if you agree.

felixfbecker commented 8 years ago

PS: I think it would be the best to only return a promise if no callback is passed. That way you don't have to expose _search, because with a callback the function would just behave like in vanialla ldapjs.

sjmeverett commented 8 years ago

Thanks for the feedback. Creating a new class to wrap the existing class is the adapter pattern and is arguably better design than duck punching the existing class.

I guess I did think about exporting it separately, I can't remember why I didn't. Seems like a good idea really.

I'm not sure what you mean about the filters. Feel free to submit a PR and we'll talk about it there!

Thanks.

felixfbecker commented 8 years ago

Working on it! Some questions though: