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

[distinctdan]

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

[charpour]

I'd love to have this functionality !

[sagrawal31]

@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]

@distinctdan I have merged your PR and made some changes to your code at bfd4bf6 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]

@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]

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]

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]

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]

@distinctdan #21

[distinctdan]

@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: #22