Skip to content
This repository has been archived by the owner on May 14, 2024. It is now read-only.

Replace instanceof check with duck-typing in Filter#toBer mixin #630

Merged
merged 4 commits into from
Jul 21, 2020

Conversation

rkaw92
Copy link
Contributor

@rkaw92 rkaw92 commented Jul 16, 2020

This fixes #629 where, if this package was used in 2 instances (e.g. depended upon by 2 different packages) where one passes a Client object to the other, they would not recognize each other's BerWriter instances and throw an error on .search() with filters.

@rkaw92
Copy link
Contributor Author

rkaw92 commented Jul 16, 2020

Not sure what happened here with the CI:

##[error]Error: Command failed: git cat-file -p cdb4d83

Random failure?

@jsumners
Copy link
Member

@rkaw92 can you rebase to latest master? I pushed #631 to fix the CI issue.

@rkaw92
Copy link
Contributor Author

rkaw92 commented Jul 17, 2020

@jsumners That helped, thanks!

function mixin (target) {
target.prototype.toBer = function toBer (ber) {
if (!ber || !(ber instanceof BerWriter)) { throw new TypeError('ber (BerWriter) required') }
if (!ber || !isBerWriter(ber)) { throw new TypeError('ber (BerWriter) required') }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!ber || !isBerWriter(ber)) { throw new TypeError('ber (BerWriter) required') }
if (isBerWriter(ber) === false) { throw new TypeError('ber (BerWriter) required') }

And make the undefined check part isBerWriter.

Copy link
Member

@UziTech UziTech Jul 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should check for null in isBerWriter otherwise the error Uncaught TypeError: Cannot read property 'startSequence' of null will be thrown with this change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's the coerced check for !ber. I just want to see it in the check function instead as another inlined condition.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, I've just moved the falsiness check into the function to shorten the condition. Is this OK?

lib/filters/filter.js Outdated Show resolved Hide resolved
Co-authored-by: James Sumners <james@sumners.email>
Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Will merge and release when I am able.

@jsumners jsumners merged commit 9d69b5f into ldapjs:master Jul 21, 2020
@jsumners
Copy link
Member

⚠️ This issue has been locked due to age. If you have encountered a recent
problem that seems to be covered by this issue, please open a new issue.

Please include a minimal reproducible example
when opening a new issue.

@ldapjs ldapjs locked as resolved and limited conversation to collaborators Mar 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

instanceof check fails in filter.js
3 participants