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

better error message #8913

Merged
merged 2 commits into from Apr 24, 2017
Merged

better error message #8913

merged 2 commits into from Apr 24, 2017

Conversation

chenshay
Copy link
Contributor

adding message name in the error message

@chenshay chenshay requested a review from dvoytenko April 24, 2017 21:00
@@ -274,8 +274,8 @@ export class Messaging {
handler = this.defaultHandler_;
}
if (!handler) {
throw new Error(
'Cannot handle request because handshake is not yet confirmed!');
throw new Error('Cannot handle request ' + message.name +
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to use args, like this:

const error = new Error('Cannot handle ....');
error.args = request.name;

This way, we will be able to group errors by the message "Cannot handle..." and get a good total number, but still will be able to find the actual request name if we need to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@chenshay chenshay merged commit 6bf3259 into ampproject:master Apr 24, 2017
@chenshay chenshay deleted the error branch April 24, 2017 22:09
DarXector pushed a commit to DarXector/amphtml that referenced this pull request Apr 25, 2017
* better error message

* error
mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017
* better error message

* error
KenneyE pushed a commit to spotxchange/amphtml that referenced this pull request May 3, 2017
* better error message

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

Successfully merging this pull request may close these issues.

None yet

3 participants