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

Emit error event if other error events are not listened to #704

Merged
merged 2 commits into from
Feb 24, 2021

Conversation

UziTech
Copy link
Member

@UziTech UziTech commented Feb 19, 2021

Emit error event if other error events are not listened to.

Currently if someone wants to catch client errors they have to subscribe to multiple error events

client.on("error", (err) => {
  assert.ifError(err);
});
client.on("setupError", (err) => {
  assert.ifError(err);
});
client.on("connectError", (err) => {
  assert.ifError(err);
});
client.on("connectTimeout", (err) => {
  assert.ifError(err);
});
client.on("connectRefused", (err) => {
  assert.ifError(err);
});

This PR will allow them to just subscribe to the error event to catch all of them but still allow them to subscribe to more if they want to.

TODO:

  • Add Tests

@UziTech
Copy link
Member Author

UziTech commented Feb 19, 2021

@jsumners I wanted to get your input on this before I go further. I wasn't sure if this would be too much of a breaking change.

If they weren't listening to any errors before a connectRefused, for example, would just fail silently. With this PR if there are no listeners for the error or connectRefused event than the node process will crash because of the error (Which I think is the right thing to do anyway).

@jsumners
Copy link
Member

Seems fair to me. I could argue that this "fixes" a hole in the error handling and therefore isn't a major change.

@UziTech UziTech marked this pull request as ready for review February 23, 2021 23:27
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

Should we try to squeeze some docs in somewhere?

@UziTech
Copy link
Member Author

UziTech commented Feb 23, 2021

It looks like only connectTimeout and connectRefused don't emit an error already if the aren't listened to. The other errors (setupError and connectError) also emit an error as well as the specific event.

@jsumners
Copy link
Member

Ship it.
giphy

@UziTech
Copy link
Member Author

UziTech commented Feb 23, 2021

Ya, I will add docs to this before merging.

@UziTech
Copy link
Member Author

UziTech commented Feb 24, 2021

@jsumners what do you think about the added docs?

@jsumners
Copy link
Member

LGTM

@UziTech UziTech merged commit f247fba into ldapjs:master Feb 24, 2021
@UziTech UziTech deleted the emitError branch February 24, 2021 14:46
@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.

None yet

2 participants