-
Notifications
You must be signed in to change notification settings - Fork 8
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for taking this on. It is really appreciated.
My initial feedback won't really work as line comments, so I'm going to do it in this top-level post.
We are going to need to deprecate the subX
fields in the same manner as:
Lines 24 to 30 in e70a7cb
/** | |
* @deprecated 2022-06-26 Use `parseString` instead. | |
*/ | |
parse: (string) => { | |
deprecations.emit('LDAP_FILTER_DEP_001') | |
return parseString(string) | |
}, |
That's not the best example to follow because it isn't tested. So take a look at the following links for what we should really do:
- https://github.com/ldapjs/messages/blob/6ce6e35dee7a188d8154537831f2f5c3b59493af/lib/ldap-message.js#L26-L30
- https://github.com/ldapjs/messages/blob/6ce6e35dee7a188d8154537831f2f5c3b59493af/lib/ldap-message.test.js#L29-L56
- https://github.com/ldapjs/messages/blob/6ce6e35dee7a188d8154537831f2f5c3b59493af/lib/messages/abandon-request.js#L42-L50
- https://github.com/ldapjs/messages/blob/6ce6e35dee7a188d8154537831f2f5c3b59493af/lib/messages/abandon-request.test.js#L46-L67
After factoring in that pattern to this change, we'll review any other items.
Thanks for your feedback! I have added and tested the deprecation messages. |
There was a problem hiding this 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.
Thank you. Thankfully we won't need to keep this patch around too long since Node.js 16 is EOL this September.
This PR closes #4