Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: emit error instead of logging #4

Merged
merged 4 commits into from
May 26, 2021
Merged

feat: emit error instead of logging #4

merged 4 commits into from
May 26, 2021

Conversation

officialpiyush
Copy link
Contributor

Right now, the developers don't have any options ( other than logs ) to know if a post call failed.

Changes

  • error event is emitted instead of logging

@officialpiyush
Copy link
Contributor Author

assuming this follows semantic versioning, I'm updating the package version to 1.2.0

@jpbberry
Copy link
Owner

I'd prefer that you check for the error event actually being listened to. And if it's not error to console. The package is made for simple users so just completely disregarding all errors if they forget to add the listener seems a bit counter intuitive.

@officialpiyush
Copy link
Contributor Author

officialpiyush commented May 17, 2021

What do you think about adding a config option? That would make more sense tbh. something like logging - enabled by default

@jpbberry
Copy link
Owner

I'd rather you do

if ('error' in this._events) {
  the.emit('error', ....
} else console.error....

@officialpiyush
Copy link
Contributor Author

gotcha!

@anishshobithps
Copy link

I think it would be better also to include error in this

export interface BasePoster {
	on(event: 'posted', listener: (stats) => void);
	on(event: 'error', listener: (Error) => void);
}

Copy link

@anishshobithps anishshobithps left a comment

Choose a reason for hiding this comment

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

LGTM!

@jpbberry jpbberry merged commit 6623197 into jpbberry:master May 26, 2021
@jpbberry
Copy link
Owner

This was merged but also accompanied with a breaking importing change in v2.0.0, you can check that here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants