feat(analytics): Updated reserved event name list

[liamjones]

Description

This PR updates the docs and code with the current reserved event name list from https://firebase.google.com/docs/reference/android/com/google/firebase/analytics/FirebaseAnalytics.Event

I didn't understand the reserved analytics event name ad_activeiew in the docs and when I checked it against the list I found that it was spelt wrong, it's meant to be ad_activeview. While checking that I also realised that the reserved event name list has changed and more items are now included.

I'm not 100% sure if I should have marked this as a breaking change or not. I haven't changed the logEvent function's parameters itself but the function will now throw Error in some situations where a previous version of the library wouldn't (although, I assume if someone had previously logged with one of the reserved event names that weren't in the list Firebase would have just discarded the event anyway).

Related issues

Release Summary

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

The existing test for reserved event names on logEvent only tests one name (session_start - https://github.com/invertase/react-native-firebase/blob/master/packages/analytics/__tests__/analytics.test.ts#L121) so I've not updated the tests in this PR currently. Let me know if you want the test changed for an it.each which goes across the whole list.

The JS tests failed for Firestore but were failing before I made edits so I don't believe I've affected them.

The Android e2e tests pass. The iOS e2e tests were failing compilation locally for me due to nvm and I'm afraid I don't currently have time to dig into resolving that while keeping my other dev stuff that uses nvm working. Nothing I've done should be platform-specific so I hope the Android ones passing locally is sufficient.

I'm not sure if the spellcheck dictionary needs an update for this PR. I couldn't yarn lint:spellcheck because I don't have the spellchecker binary and I don't know what app I need to install for that (couldn't find it listed anywhere in the contributing docs). Happy to update the PR (and raise a new PR for updating the contributing docs) if I know what spellchecking app I need.

Reserved event name app_exception was commented out from the list before I did my updates so I've kept it commented out in the new list. Likewise, it wasn't mentioned in the docs so I've not added it. This looks to be because of this piece of code in the crashlytics module: https://github.com/invertase/react-native-firebase/blob/master/packages/crashlytics/lib/handlers.js#L100. Would you like a more specific solution to be implemented here so it can still throw an error for normal consumers? I was debating having it allow app_exception if the params are fatal and timestamp (which is how the crashlytics module calls it) but throw Error otherwise?

🔥

[vercel]

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

react-native-firebase – ./

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/Ny6CEhZFWPvXveJh7XX4VVHWifzT
✅ Preview: https://react-native-firebase-git-fork-liamjones-updat-e18cb3-invertase.vercel.app

react-native-firebase-next – ./website_modular

🔍 Inspect: https://vercel.com/invertase/react-native-firebase-next/xv4K1UfCYYR9gD6ZpFaNjmXZAv6Q
✅ Preview: Canceled

[codecov]

Codecov Report

Merging #5630 (011f716) into master (2545349) will decrease coverage by 19.37%.
The diff coverage is n/a.

❗ Current head 011f716 differs from pull request most recent head 084045e. Consider uploading reports for the commit 084045e to get more accurate results

@@              Coverage Diff              @@
##             master    #5630       +/-   ##
=============================================
- Coverage     73.08%   53.72%   -19.36%     
- Complexity        0      632      +632     
=============================================
  Files           109      208       +99     
  Lines          4487    10076     +5589     
  Branches        957     1543      +586     
=============================================
+ Hits           3279     5412     +2133     
- Misses         1129     4381     +3252     
- Partials         79      283      +204     

[mikehardy]

This is motivating for me to fix the nvm issue and spellchecker issue for contributing actually, I use nvm and solve the problem with a symlink in bash profile on logins to current default to local bin, spelling I let ci fuzz for me but that's a weak effort 😂

The tests should pass though, ci will show jt

Should not be breaking, the underlying sdk actually throws if you touch a reserved name iirc? A separate test direct to the native module can confirm that, there are a few tests in app for config that show direct native access to verify things

Anyway I'll scan this next batch of work I do here. Thanks a bunch for posting this!

[mikehardy]

Hmm - I added this to packages/analytics/e2e/analytics.e2e.js:

  describe.only('logReservedEventName()', function () {
    it('should reject reserved event name at native level', async function () {
      try {
        await firebase.analytics().logEvent('ad_activeview');
        console.error('it took it');
      } catch (e) {
        return Promise.resolve();
      }
      throw new Error('should have rejected');
    });
  });

...and it took it - there was no rejection, so this will be a breaking change as is.

It doesn't feel worth it to issue a breaking change just this moment, just to update the list.

I can think of two options to queue this, one starts giving people warning right now then breaks which is to make a new array of "warningReservedWords" that's checked and emits a console.warning but allows it, and one just queues the PR for later

I much prefer the idea of warning now, with a comment // BREAKING that will show up with a grep for breaking changes, so that the code is "live" and there's no need to pay attention to a branch of code, but that implies more work in this PR to add a new array etc. I don't want to commit you to more work because hey, open source / volunteer :-), but if you're willing it would be great.

What do you think?

[liamjones]

I much prefer the idea of warning now, with a comment // BREAKING that will show up with a grep for breaking changes, so that the code is "live" and there's no need to pay attention to a branch of code, but that implies more work in this PR to add a new array etc. I don't want to commit you to more work because hey, open source / volunteer :-), but if you're willing it would be great.

Sure, it might take me a little while to find the time but I'm up for doing it. :)

[mikehardy]

If it's a hassle don't worry about it - less code means less bugs and I can just keep it in the queue. Not sure how long until there will be a real breaking change release but this shouldn't really decay much in the interim

[mikehardy]

Finally time to do a breaking change here and merge this one! Thanks again for contributing this, I just compared it to the list of reserved ones again just in case and it is spot on