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

Update Firebase SDK Version #796

Closed
briantq opened this issue Aug 8, 2018 · 18 comments
Closed

Update Firebase SDK Version #796

briantq opened this issue Aug 8, 2018 · 18 comments

Comments

@briantq
Copy link
Contributor

briantq commented Aug 8, 2018

Current SDK version is 5.5.0 and the one in the plugin is 4.13.0. In the 5.x release there have been enhancements to A/B Testing, Remote Config, Performance Management and other components with bug fixes.

@soumak77
Copy link
Contributor

PR is welcome to upgrade to 5.x

@briantq
Copy link
Contributor Author

briantq commented Aug 30, 2018

@soumak77 I didn't see any information on contributing. Are there any tests to ensure after I replace the libraries that the APIs didn't break the code? Currently, I am only using Messaging which should be easy enough to test with my application, but unsure how to test the other aspects of Firebase. Thanks!

@soumak77
Copy link
Contributor

@briantq right now there are only tests to verify the builds are working with various platforms/versions. Currently there are no tests to verify runtime functionality, so you'll need to perform those tests manually. If any PR were to break someone's app, I will revert those changes and publish a new version.

@soumak77
Copy link
Contributor

@briantq I'll actually take care of updating the SDK. I'm planning to make a new repo with the iOS SDK source so that it doesn't need to be re-downloaded whenever a new version of this repo is published. This will be a temporary fix until cocoapods integration is added.

@soumak77
Copy link
Contributor

soumak77 commented Aug 30, 2018

I've refactored the repo to remove the iOS SDK and moved the source to https://github.com/soumak77/firebase-ios-sdk. The build is working with version 4.13.0 of the SDK.

I've added version 5.7.0 of the SDK (NOTE: I had to publish this as 5.7.1 due to an issue with a missing file in 5.7.0). This latest version can be downloaded by changing the firebase-ios-sdk version in package.json to ^5.0.0 and then running the command npm install.

Unfortunately, the iOS build is broken with the latest SDK due to changes in file and folder names. You can run the command npm test:ios to verify the build is working. I'll need help from someone in the community to get the build working with 5.x of the SDK.

@soumak77
Copy link
Contributor

On second thought, I'm not sure using npm to install the SDK is the best approach. This was intended to just be a workaround until cocoapods integration was complete anyway, so I'm going to revert the changes to use https://github.com/soumak77/firebase-ios-sdk and keep the SDK as part of this repo.

@briantq
Copy link
Contributor Author

briantq commented Aug 31, 2018

@soumak77 Sounds reasonable. Thanks for trying to do the refactoring.

Is cocoapod support not complete? I saw it in the documentation. Please pardon my ignorance as I recently started with Cordova.

My background is with Android, but I can take a stab at trying to run the iOS tests and seeing what I can come up with. Is there a branch or easy way to set up the environment?

@soumak77
Copy link
Contributor

@briantq please check out #672 (comment). @christocracy already did the work to integrate cocoapods into this repo, however, many changes have been made to master since he did that and the merge is no longer strait forward. Hopefully his work is easy to follow so that you, or someone else, can make the necessary changes to master.

I would start by integrating the changes to use cococapods with v4.13.0 of the SDK. Once that work is complete and merged, you can focus on updating the SDK to the latest version.

@briantq
Copy link
Contributor Author

briantq commented Sep 5, 2018

@soumak77 I was running into some Messaging issues and wanted to see if the issue would be resolved with a newer version of the library, so I started upgrading the library. When I downloaded the latest version fo the frameworks, I found there were a number of new frameworks, many around ML. After resolving an API change, I got the project to build but it died right away talking about App Invites not being configured correctly.

I don't have much familiarity with App Invites, so wanted to see if you supported them? If not, I might suggest trimming the contents of the frameworks you ship to reduce the support costs of having to configure frameworks your don't support. Likewise, since the ML support is new that is another thing that seems like it would make sense to trim out. It would reduce the download size (700MB+ for ML support).

Thoughts?

@soumak77
Copy link
Contributor

soumak77 commented Sep 5, 2018

Yes, I would suggest only updating the frameworks that existed in 4.x. I did notice the Crashlytics folder was renamed to just Crash in 5.x. You might be able to get away with just renaming that folder, otherwise you'll have to update some source code to point to the new folder name.

@soumak77
Copy link
Contributor

soumak77 commented Sep 5, 2018

Another issue I remember having to deal with when trying to refactor the SDK and update to 5.x was there is one file which is over 100mb and GitHub does not allow checking in files over that size. NPM does however allow those file sizes, so I had to exclude the file in .gitignore, but allow it in .npmignore. Unfortunately this means that anyone trying to play with the source from GitHub will need to manually download that file from NPM to do any local testing. Those just installing the plugin should be okay since it is downloaded from NPM by cordova during the install process.

We really need to switch to cocoapods so we don't need to check-in the SDK, but that is a large undertaking that no one from the community is willing to deal with at this time.

@soumak77
Copy link
Contributor

soumak77 commented Sep 5, 2018

Oh man, I just realized this might also break the iOS travis tests since the tests rely on code only checked into GitHub. We can deal with that later though if you're able to get the SDK updated to 5.x

@briantq
Copy link
Contributor Author

briantq commented Sep 5, 2018

@soumak77 Do you use all the frameworks? I know you use Messaging (as I use that personally). I saw some references to Analytics. I believe the verifyPhoneNumber comes from Auth. I saw some references in the code base to Crashlytics, Performance and and Analytics (previously mentioned).

Does that mean you don't use ABTesting, AdMob, Database, DynamiclLinks, Firestore, Functions, InAppMessagingDisplay, Invites, RemoteConfig and Storage? If so, can we remove them instead of trying to work around configuring them to get the app to launch (as in Invites).

By limiting the frameworks included, we can reduce the download size, not have to worry about the large file size (ML and Admobs are the only ones with concerning sized files), and minimize upgrade cost by not having to configure unnecessary features.

Before I tried to do that, I wanted to confirm which frameworks are in use so I can prune accordingly.

@soumak77
Copy link
Contributor

soumak77 commented Sep 5, 2018

I would remove any frameworks not used. You should find out pretty quickly whether or not a framework is used by whether or not the iOS build is broken. You can verify by running:

npm run test:ios

If the tests pass, the build is working

@briantq
Copy link
Contributor Author

briantq commented Sep 5, 2018

@soumak77 Thanks!

@briantq
Copy link
Contributor Author

briantq commented Sep 5, 2018

@soumak77 I updated the frameworks, pruned out the ones that aren't needed, updated an API method that changed, and was able to successfully build my app when I installed the dependency from my repo.

You mentioned running npm run test:ios but when I did it failed in the code signing phase.

Please let me know if there are other tests I should run or if something is not done correctly.

@soumak77
Copy link
Contributor

soumak77 commented Sep 5, 2018

@briantq every PR triggers the test suite to run. Your PR failed for iOS when using cordova 6.x and 7.x. You can investigate the issues by clicking the link in the PR on github. Any changes you make to the PR will rerun the suite, so you can try out various fixes.

@soumak77
Copy link
Contributor

soumak77 commented Sep 8, 2018

changes have been released via v2.0.0

@soumak77 soumak77 closed this as completed Sep 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants