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

feat(crashlytics): add custom message ability to javascript stack traces #4609

Conversation

wilau2
Copy link
Contributor

@wilau2 wilau2 commented Nov 24, 2020

Description

  • Add ability to override the error message. Bringing back the record custom error.
  • Alerting will be more usefull in firebase as well, it's sad to receive an Email with index.bundle:123192038123:1239810238
  • Easier triage in the the crashlytics dashboard
  • Add an initial line to the stack (the one that will be printed in the crashlytics UI, the aggregation works with this custom string as well)

Related issues

Release Summary

  • I made the parameter optional should not introduce any breaking change

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS
  • My change includes tests;
    • e2e tests added or updated in packages/\*\*/e2e
    • jest tests added or updated in packages/\*\*/__tests__
  • I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • Yes
    • No

Test Plan

image
image


Think react-native-firebase is great? Please consider supporting the project with any of the below:

@CLAassistant
Copy link

CLAassistant commented Nov 24, 2020

CLA assistant check
All committers have signed the CLA.

@vercel
Copy link

vercel bot commented Nov 24, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/jnvlkl4z0
✅ Preview: Failed

[Deployment for c691a8e failed]

@codecov
Copy link

codecov bot commented Nov 24, 2020

Codecov Report

Merging #4609 (8783a1e) into master (bda2d67) will decrease coverage by 0.10%.
The diff coverage is 33.34%.

@@            Coverage Diff             @@
##           master    #4609      +/-   ##
==========================================
- Coverage   85.42%   85.32%   -0.09%     
==========================================
  Files         109      109              
  Lines        3710     3712       +2     
  Branches      346      347       +1     
==========================================
- Hits         3169     3167       -2     
- Misses        476      479       +3     
- Partials       65       66       +1     

@wilau2 wilau2 changed the title 🔥fix: crashlytics custom js error display in firebase ui 🔥 fix: crashlytics custom js error display in firebase ui Nov 24, 2020
@wilau2 wilau2 changed the title 🔥 fix: crashlytics custom js error display in firebase ui 🔥 fix(crashlytics): custom js error display in firebase ui Nov 24, 2020
@wilau2 wilau2 changed the title 🔥 fix(crashlytics): custom js error display in firebase ui fix(crashlytics): custom js error display in firebase ui Nov 24, 2020
@mikehardy
Copy link
Collaborator

Cool! It is sad to see the bundle line numbers and character numbers ;-)

This seems fine, just need to figure out a couple CI checks

  • Vercel is unrelated
  • Android E2E is also unrelated, it's a flaky test I need to squish from a different PR
  • Typedoc generation failure is new - not sure what's going on there and I need to triage

Holding off on approval until I know more on the typedoc problem - reasonably sure it's unrelated though, this all looks fine as I read the diffs

@mikehardy mikehardy added Workflow: Needs Review Pending feedback or review from a maintainer. Workflow: Pending Merge Waiting on CI or similar labels Nov 24, 2020
@mikehardy
Copy link
Collaborator

I have triaged all the CI failures and they are fixed on main branch now. Vercel and Typedoc generate were unrelated to this PR but were related to each other, fixed in the commit that fixed this #4600

Android E2E flake fixed directly also e1b154a

This should be good to go, will rescan

@mikehardy mikehardy changed the title fix(crashlytics): custom js error display in firebase ui feat(crashlytics): add custom message ability to javascript stack traces Nov 26, 2020
Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Ok, LGTM - thanks!

@mikehardy mikehardy merged commit afaa95d into invertase:master Nov 26, 2020
@mikehardy mikehardy removed Workflow: Needs Review Pending feedback or review from a maintainer. Workflow: Pending Merge Waiting on CI or similar labels Nov 26, 2020
@WookieFPV
Copy link

Awesome PR, I really like this feature.

How do you feel about going even a step further?
Instead of it could also send a customizable string.
crashreporting

@mikehardy
Copy link
Collaborator

I am always open to "reasonable" PRs (where yes "reasonable" is fuzzy, but in general I mean as, maintains compatibility with firebase-js-sdk APIs, is not a breaking change, is well tested and documented, helps dev etc. This is an area that is kind of specific to us as we have react-native specific issues with stacks because of bundling, so I'm open to developer experience help in this area

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.

4 participants