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

Reverting changes to make Firebase initialize lazily by default #832

Merged
merged 2 commits into from
Sep 12, 2018

Conversation

briantq
Copy link
Contributor

@briantq briantq commented Sep 8, 2018

THIS PR NEEDS TESTING. I don't personally leverage Analytics or Performance so can't confirm if they are working as intended

This PR is to revert the changes to have Crashlytics, Analytics and Performance all initialize lazily. This should revert the previous behavior prior to PR #784.

NOTE: I believe PR #784 did not properly implement Crashlytics on iOS, therefore I don't think this PR will make Crashlytics work on iOS currently. I will work on another PR for that.

The changes for this were quite significant. We should do a reverse commit after merging this and put it on another branch. I believe the lazy initialization was a fairly complete implementation. It only requires some configuration to dynamically initialize features, along with some documentation on how to use it.

@soumak77
Copy link
Contributor

soumak77 commented Sep 8, 2018

I'm fine making these changes to the master branch only. I have created a v1 branch for fixes that need to be made to v1 of the plugin so that users aren't forced to adopt v2 right away. Fixes made to v1 will also be merged into master. Any other new features should be made to master only and released as part of v2 of the plugin. Let me know if you disagree.

@briantq
Copy link
Contributor Author

briantq commented Sep 8, 2018

@soumak77 Ah, the great merge debate. I am a fan of merging all changes to master and then cherry picking/modifying fixes to go to release branches. This ensures there is no chance of a regression (change to release branch that someone forgets to merge to master -> next release has regression).

I would prefer to have at least one person test it before it goes into master, so we have some confidence we are in a semi-good state.

Also, I believe Issue #828 is affected by the change to lazy init Analytics. If this does check out, this would be a good candidate to include v1 so people who upgrade don't have to change their code to get the same behavior in these features.

@soumak77
Copy link
Contributor

soumak77 commented Sep 8, 2018

I definitely agree we need PRs tested before being merged. Hopefully we don't need to maintain a separate v1 branch for very long. Once we get v2 stable, we can write a migration guideline in the README and then we don't need to support v1 anymore.

@newuser44
Copy link

What is the plan to get this merged in? Or did this get merged in? Which version?

@briantq
Copy link
Contributor Author

briantq commented Sep 11, 2018

@newuser44 Once this gets tested, we can merge this. The issue I have is that I don't currently leverage Crashlytics or Analytics in my project so I don't know if this fixes the issue.

It takes only a few minutes to update the plugin to test. The following commands will remove the current version and install the propose changes (which should initialize Crashlytics, Analytics, Performance by default)

  1. cordova plugin remove cordova-plugin-firebase
  2. cordova plugin add https://github.com/briantq/cordova-plugin-firebase.git#revert-lazy-init
  3. cordova prepare

If you could post back whether it worked or not, that will help us confirm the validity of the fix and/or if we need to make any changes.

@newuser44
Copy link

I might some time this afternoon and can try it. We knew that the Crashlytics or what ever wasn't supported by this, so we didn't try to implement it. We do have Analytics (page name and events) and Performance (start trace stuff).

@newuser44
Copy link

Do you guys know if there is a way to turn on DebugView for Android? Doing Debug view on ios everything looks good. Seeing a steady stream of data coming in. Would like to do the same for android, but I push the apk to the phone.

@soumak77
Copy link
Contributor

@newuser44 You should be able to debug using an emulator in android studio. Here is a link to their debug info: https://developer.android.com/studio/debug. At a minimum you can view the system log to verify no errors are reported about not being able to initialize firebase or any firebase services.

@soumak77
Copy link
Contributor

If you're using ionic, they also provide a built-in way to debug android apps via the command: ionic run android --device -l -c

@newuser44
Copy link

My dev env is on a Linux box inside virtualBox. Not sure if its my computer or the virtualbox itself but to run android studio I need certain processor feature that Android Studio says I don't have.

Day after I'm able to look at all the data. From as far as I can tell I'm getting the data I expect.
I see Event Data with screen_view - screen_name for both android and ios.
I'm seeing other Event data with parameters come through for both.
I see performance data also.
I am not able to say anything on Crashlytics because we never implemented it.

@soumak77
Copy link
Contributor

@briantq I'm ok with merging this now. Let me know if you still prefer more testing.

@soumak77
Copy link
Contributor

@briantq could you also create an identical PR which adds this change to the v1 branch instead of master? This way I can publish v1.1.5 and v2.0.1 with this fix.

@newuser44
Copy link

I just noticed for performance data I can see data for ios, I even remember seeing some of the calls to websites when I was debugging. But I don't see any preformant network data for android. Does that have to be set up separately? That might even be related to this branch, never gotten that deep in some of this data before.

@soumak77
Copy link
Contributor

@newuser44 this sounds similar to #697. I don't think that issue will prevent merging this PR, but definitely needs to be investigated further.

@briantq
Copy link
Contributor Author

briantq commented Sep 12, 2018

@soumak77 Sure. We probably also want to include the Crashlytics build step fix also.

@soumak77
Copy link
Contributor

@briantq I would prefer to just keep Crashlytics fixes as part of v2 so we don't need to maintain v1 for very long. Crashlytics never really worked in v1 and v1 doesn't have the updated SDK, so I'm not sure if it would even work if we did add the build step. If you disagree, I'm fine adding the fix to v1.

@soumak77 soumak77 merged commit e16968a into arnesson:master Sep 12, 2018
@soumak77 soumak77 mentioned this pull request Sep 12, 2018
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.

3 participants