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

Store#emit: if storage.emit throws an error, log it instead of hiding it #234

Merged
merged 1 commit into from
Apr 9, 2020

Conversation

msssk
Copy link
Contributor

@msssk msssk commented Feb 18, 2020

Store#emit: if storage.emit throws an error, log it instead of hiding it

Fixes #187

@xmedeko
Copy link

xmedeko commented Feb 19, 2020

This is not fix, just workaround. E.g. build by the Closure compiler removes all console logs. Or a developer may have own uncaught error handling, etc.
The real fix is to remove the return in the finally clause.

@xmedeko
Copy link

xmedeko commented Feb 20, 2020

Or, at least allow to set some callback for custom error handling.

@msssk
Copy link
Contributor Author

msssk commented Feb 28, 2020

@xmedeko thank you for thinking about this issue and how best to solve it. dstore's design follows the rationale below:

build by the Closure compiler removes all console logs

It is a developer's responsibility to consider the ramifications of altering the dstore code. dstore has for years made use of console to report errors and warnings. If dstore code is altered to remove all console logs then its ability to report errors will be removed.

a developer may have own uncaught error handling

It is a developer's responsibility to handle errors in their own code. dstore is designed to be resilient and not fail unnecessarily due to errors in external code.

The real fix is to remove the return in the finally clause

This would allow external code to unnecessarily disrupt dstore's functionality.

allow to set some callback for custom error handling

Listeners to dstore event notifications are under the control of the developer already, so custom error handling can (and should) be implemented in that code; it is not dstore's responsibility.

Store#emit is a one-way communication. It is simply a notification, not a hook to allow external code to alter internal dstore logic. All uses of emit in dstore ignore the return value and the event object. A failure in an event listener should not disrupt the functioning of dstore.

@xmedeko
Copy link

xmedeko commented Feb 28, 2020

@msssk Thanks for explanations of dstore concepts. I am new to dstore, but have experience with many frameworks in different languages.

It is a developer's responsibility to handle errors in their own code. dstore is designed to be resilient and not fail unnecessarily due to errors in external code.

I find as a wrong concept. A framework should help a programmer to report uncaught errors, so a programmer does not have do to it by himself. (How many dstore apps do it properly?)

Anyway, I understand you stick to backward compatibility.

@msssk
Copy link
Contributor Author

msssk commented Feb 29, 2020

A framework should help a programmer to report uncaught errors

Agreed on this point, which is why the initial implementation that silently swallowed the error is being fixed to report the error in the console. So the error will now be reported with this update, but dstore will continue to be resilient to errors in code external to dstore's functionality.

@xmedeko
Copy link

xmedeko commented Mar 1, 2020

Yes, this commit is an improvement, no doubt. Just let me show you how simple is to do a customizable logging in a library, see #239

@msssk msssk merged commit faaa99b into SitePen:master Apr 9, 2020
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.

Event handler errors are silently swallowed
3 participants