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

Move from Firebase Crash to Firebase Crashlytics and stop opt-in users #784

Merged
merged 7 commits into from
Aug 30, 2018
Merged

Conversation

gvieduc
Copy link
Contributor

@gvieduc gvieduc commented Jul 27, 2018

Hi,

I worked on your plugin to use Firebase Crashlytics instead of Crash because it stop on 9/9.
I succeed to use Firebase Crashlytics with Google Service and without Fabric API key.
Moreover I modified the way Firebase is initialized, opt-in is disabled by default to be GDPR compliant. Now you'll have to initialize each Firebase services to activate them so we'll can let user to choose to not use Google's services.

Lot's of files and ideas come from .

Disclaimer : I'm beginner with Cordova plugins and it's the first time I develop in Objective-C ! Every advise on the way I did this are welcome.

Thanks for reading me,

Guillaume (or William in english :) )

@ndrake ndrake mentioned this pull request Aug 3, 2018
@StephanMeijer
Copy link

Is this a working solution for Android?

@StephanMeijer
Copy link

Lot of files changed..?

@gvieduc
Copy link
Contributor Author

gvieduc commented Aug 8, 2018

We're using it in development for now, it seem to be working. It only connect if you initialize Firebase and Firebase's Crashlytics was enabled after using it.
See my fork for last commit for iOS.

@StephanMeijer
Copy link

Do you have any code examples on initializing Firebase?

@gvieduc
Copy link
Contributor Author

gvieduc commented Aug 8, 2018

No but take a look at the file : www/firebase.js
I added initialization functions.

exports.initFirebase = function (success, error)
exports.initCrashlytics = function (success, error)
exports.initAnalytics = function (success, error)
exports.initPerformance = function (success, error)
exports.initRemoteConfig = function (success, error)

@StephanMeijer
Copy link

Thanks

@StephanMeijer
Copy link

Can you undo your stylechanges, please? (e.g. spacing)

@gvieduc
Copy link
Contributor Author

gvieduc commented Aug 9, 2018

I'll do it, but not for now, I don't have the time to.

</config-file>
<config-file parent="/resources" target="res/values/strings.xml">
<string name="google_api_key">@string/google_api_key</string>
</config-file>

Choose a reason for hiding this comment

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

Why the removement if those values?

};

function updateStringsXml(contents) {

Choose a reason for hiding this comment

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

Isn't this out of scope of the PR?

@gvieduc
Copy link
Contributor Author

gvieduc commented Aug 27, 2018

Hi,
I used scripts/lib/android-helper.js from the official Crashlytics plugin. It add "apply plugin: 'com.google.gms.google-services'" in the top of the main Gradle file and so use google-service.json to read these values.
As what I've seen it's working in this configuration but I'm listening to all of your ideas.

Guillaume

@BluebambooSRL
Copy link

Hi @StephanMeijer @soumak77 could you please merge?
Thanks in advance
Federico

@bensinclair
Copy link

+1 on merging

@StephanMeijer
Copy link

Merging would be nice, indeed.

@Victordgrandis
Copy link

+1 on merging

@soumak77 soumak77 merged commit f237715 into arnesson:master Aug 30, 2018
@soumak77
Copy link
Contributor

PR has been merged and is available in v1.1.0

@briantq
Copy link
Contributor

briantq commented Sep 6, 2018

@soumak77 I am 99% sure this breaks crash reporting on iOS. Looking at the documentation, Crashlytics requires a build phase to be added and it isn't added. By removing the Firebase Crash framework, iOS has no working crash reporting framework.

Do PRs typically have documentation updates also? By changing the behavior of the way Firebase initializes, along with the new JS APIs, this will likely cause problems when people adopt the plugin and can't understand why it isn't working.

@soumak77
Copy link
Contributor

soumak77 commented Sep 6, 2018

@briantq in hindsight I should have released these changes under a major release (2.0.0) instead of a minor release (1.1.0) due to all the breaking changes. PRs should add documentation changes when necessary, however, many people requested this PR merged so I assumed it had been tested by the community, but apparently it wasn't fully tested on all platforms.

I don't have time to manually test every PR, so I am relying on the community to test and verify that newly released versions and proposed PRs are working as desired. Worst case, users can hardcode the version of the plugin that is working for them. I understand how annoying this can be, but that is just part of dealing with open source projects.

@soumak77
Copy link
Contributor

soumak77 commented Sep 6, 2018

@gvieduc could you please provide a PR with updates to the README so that others are able to understand exactly what is required to get Crashlytics working on android and iOS? It appears there are some manual steps required that are not easy to figure out.

If someone else from the community could provide this info, I'll gladly accept the PR

@soumak77
Copy link
Contributor

soumak77 commented Sep 6, 2018

It appears this PR removed the auto-init functionality. Would any be able to help investigate and figure out what code needs to be added back to fix this? I would love to accept a PR and push out a new patch release so that those adopting v1.1.x don't have to change their code to get the plugin to work as it used to work (i.e. no breaking changes).

If needed, we can add a cordova configuration variable which disables the auto-init functionality.

@StephanMeijer
Copy link

Is the functionality in this PR still working and merged into the master-branch? cc @soumak77

@soumak77
Copy link
Contributor

@StephanMeijer yes this has been merged though it only worked on android. #836 fixed this on iOS. We still need #832 tested then I'll release a new version.

briantq added a commit to briantq/cordova-plugin-firebase that referenced this pull request Sep 12, 2018
@soumak77 soumak77 mentioned this pull request Sep 12, 2018
@briantq briantq mentioned this pull request Sep 15, 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.

None yet

7 participants