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

Replacing console error with an onError option #20

Merged
merged 2 commits into from
Jan 5, 2018

Conversation

ekynoxe
Copy link
Contributor

@ekynoxe ekynoxe commented Nov 22, 2017

I had a first stab a #19.

This is ultra simplistic and very simple, but it does the initial job of delegating error handling the the user.

One thing I didn't really like was the check for process.env.NODE_ENV which might not be available, therefore you'd potentially throw errors in production code anyway.

I've only done the makeDecryptor method so far, but maybe we need to add this to the makeEncryptor method too?

src/helpers.js Outdated
if (process.env.NODE_ENV !== 'production') {
console.error(err);
}
onError && onError(err);
Copy link
Owner

@maxdeviant maxdeviant Nov 23, 2017

Choose a reason for hiding this comment

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

I would prefer this check to be typeof onError === 'function', as that would prevent it from trying to call some truthy non-function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right. I'll update this.
Do you think this should also be added to the makeEncryptor method as well?

@maxdeviant
Copy link
Owner

@ekynoxe Yea, the process.env.NODE_ENV check isn't the greatest, and delegating the error handling to the consumer makes it their choice how they want to handle it.

I think the other place where we should use onError is here instead of throwing an error.

@ekynoxe
Copy link
Contributor Author

ekynoxe commented Nov 24, 2017

Regarding your comment on the sync.js file, is it strictly necessary? I believe this will be caught by the try/catch where I did my original update, won't it?

@maxdeviant maxdeviant merged commit a9aef7a into maxdeviant:master Jan 5, 2018
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.

2 participants