Error in event handlers when handling JSON is caught and discarded

[gmrchk]

Do you want to request a feature or report a bug?
Bug

What is the current behavior?
Any error in event handlers when handling JSON is caught and discarded by the library. If the error repeats with the string version of the input, only that is displayed.
This try-catch is the cause.

pusher-js/src/core/channels/encrypted_channel.ts

Line 123 in 3e49245

emitJSON(eventName: string, data?: any): Dispatcher {

The library will catch any error in the event handler, and emit the event once again with a string. In case the handler contains any check for the format of input and doesn't run with the string input, it won't error at all.

In general, being benevolent about the format (try JSON, fallback to string) might not be an optimal solution, but that's just my opinion.

If the current behavior is a bug, please provide the steps to reproduce and
if possible a minimal demo of the problem via https://jsfiddle.net or similar.
Throw an error in any event handler. It will only throw an error in the second run here.

pusher-js/src/core/channels/encrypted_channel.ts

Line 127 in 3e49245

this.emit(eventName, data);

const handler = () => throw Error("Error that won't be displayed");
channel.bind(`api-${eventName}`, handler);

What is the expected behavior?
Try-catch block should only try to parse the JSON and another try-catch can be present for the actual call of the handler.

Which versions of Pusher, and which browsers / OS are affected by this issue?
Did this work in previous versions of Pusher? If so, which?
Latest, probably continuous problem.

[leesio]

@gmrchk thanks for raising this. This is definitely not good behaviour. If I've understood properly, I think #475 should resolve it. What do you think?

In general, being benevolent about the format (try JSON, fallback to string) might not be an optimal solution, but that's just my opinion.

I happen to share this opinion. Unfortunately it's a change that has a high chance of breaking lots of stuff so it would have to be in a major release.

[gmrchk]

Nice work @leesio. It should save many people tedious debugging by showing the actual error right away. It was very confusing to search for why is there even a string received instead of JSON.

I happen to share this opinion. Unfortunately it's a change that has a high chance of breaking lots of stuff so it would have to be in a major release.

Yep, definitely something to learn from in the future versions. Your fix addresses the main issue now. 👍

[leesio]

Hey @gmrchk I just released v7.0.0 which includes the fix for this. If you could test it works as expected I'll close the issue 😄

[leesio]

I’m going to close this, as it should be resolved now. If I’m mistaken please reopen