Fixed crash logging on iOS, added ability to log JS stack traces.

distinctdan commented 5 years ago

Hi, I've done these fixes for my own project and wanted to make them available to others as well.

charpour commented 5 years ago

I'd love to have this functionality !

sagrawal31 commented 5 years ago

@distinctdan I see you rebased the master. But we will get this merged once we fix the issue of the breaking change due to Firebase.

sagrawal31 commented 5 years ago

@distinctdan I have merged your PR and made some changes to your code at https://github.com/wizpanda/cordova-plugin-firebase-lib/commit/bfd4bf60f0722e8186a82d53880f17698c38f93f to make it more readable and DRY.

Also, instead of mixing the same method, I have created a new method logJSError which makes it more readable and backward compatible for the users.

Please let me know your thoughts.

distinctdan commented 5 years ago

@sagrawal31 Thanks for looking into it. However, I see some issues with your changes, and it looks like you didn't test enough before you merged because you've got typos and you broke functionality. Here are my observations:

Typo in firebase-browser: xports.logError should be exports.logError
I like moving the stack parsing to the JavaScriptException class, good idea.
However, you need to handle if stacktrace is null. As it is right now, it looks like it will throw an error if it's not passed a stack. Users may not always be passing stacks, but it's still important to use a separate class for the exception so that in firebase, it shows up as a js exception instead of a regular one. You can see that a JavaScriptException is still created if the user doesn't pass a stack, on the line Crashlytics.logException(new JavaScriptException(message));, but your new implementation looks like it will throw in this case when it hits stackTrace.length()
On the js side, it doesn't make sense to add another method because all errors logged from js are javascript exceptions, so it doesn't make sense to differentiate between "error" and "jsError", because all errors are js errors. The method I wrote is backwards compatible and follows the existing pattern in this library for optional arguments. You can look at exports.fetch or exports.verifyPhoneNumber for examples. At the end of the day, I think this library could have been designed better to put the success and error callbacks as the first arguments, and have all other arguments last, but changing that would be a breaking change. I'd ask you to please consider changing it back.

distinctdan commented 5 years ago

Oh, one other thing I forgot to do, after we get everything straightened out, we'll need to update the docs to reflect the changes.

sagrawal31 commented 5 years ago

I agree with the points you mentioned. It's true that I didn't enough and only did the sanity checks but I believed the PR coming to repositories are testing enough.

But yeah, I should have tested it thoroughly after the changes. But thanks for pointing those out. I'll make the necessary changes.

charpour commented 5 years ago

Oh, one other thing I forgot to do, after we get everything straightened out, we'll need to update the docs to reflect the changes.

Couldn't agree more that DOCs are equally important

sagrawal31 commented 5 years ago

@distinctdan https://github.com/wizpanda/cordova-plugin-firebase-lib/pull/21

distinctdan commented 5 years ago

@sagrawal31 Looks great, thanks for taking a look at it. I've also done another PR to add an example to the docs of how to use StackTrace.js to pass a stacktrace to the method: https://github.com/wizpanda/cordova-plugin-firebase-lib/pull/22

wizpanda / cordova-plugin-firebase-lib

Fixed crash logging on iOS, added ability to log JS stack traces. #8