flutterfire_desktop icon indicating copy to clipboard operation
flutterfire_desktop copied to clipboard

chore: Update dependencies and fix tests

Open davidmartos96 opened this issue 1 year ago • 16 comments

This PR changes multiple things. Unfortunately it was hard to separate them if I wanted the CI to pass.

All unit tests and e2e tests are passing on the GH Actions from my fork.

This is the summary of the changes:

1. Update the firebase dependencies

firebase_auth max version 4.6.3
Newer versions use pigeon https://pub.dev/packages/pigeon to communicate with the platforms so there is more pending work to make it work with the latest. Because of this transitively firebase_auth_platform_interface is compatible up to version 6.15.3.

firebase_core ^2.0.0 Latest version 2.15.0 is resolved fine

cloud_functions max version 4.0.13 Newer versions add support for 2nd generation functions. Same as auth, there is more pending work here. Because of this transitively cloud_functions_platform_interface is compatible up to 5.1.32.

2. Update the cloud functions typescript project

The tests from firebase_functions_dart expected some functions to exist, which apparently were removed in fe794c818952c190206dd80fa5b8b5e0a32cee7b but not the tests. Maybe tests from firebase_functions_dart can be removed and just keep the e2e tests.

3. Update to melos 3

This changes the syntax in the melos.yaml, adds the pubspec_overrides.yaml files and includes a top level pubspec.yaml file

4. Make the linter happy

This includes bumping the Dart SDK on some subprojects, as some were using functions that were not available in the declared version.

Hopefully this helps with the review. I have further changes to reuse the e2e tests from https://github.com/firebase/flutterfire but I think it would be better to go step by step :smile:

It's sad to see this project with not much maintenance, I believe it is a great solution to have some basic support for Linux and Windows while it is not official.

davidmartos96 avatar Jul 30 '23 10:07 davidmartos96

Can someone please review the PR? @pr-Mais, @Salakar , @nilsreichardt , @Ehesp, @TimWhiting , @lesnitsky , @ruicraveiro , @marb2000 , @britannio

dbebawy avatar Aug 10 '23 18:08 dbebawy

I'm not an admin here, but the changes look mostly mechanical and look good to me. Getting all the plugins fully up to latest dependencies would make a PR hard to review I'm guessing, because of the amount of required changes, so I appreciate that this just brings them the most up to date with minimal changes.

TimWhiting avatar Aug 10 '23 19:08 TimWhiting

Cannot use this plugin anymore with latest Firebase_core. I guess this PR would fix that?

johannesvuorinen avatar Aug 22 '23 11:08 johannesvuorinen

Cannot use this plugin anymore with latest Firebase_core. I guess this PR would fix that?

That's correct. You will need to use dependency overrides for each of the firebase_desktop and _dart dependencies and keep the constraints described above. For instance maximum allowed version for auth is 4.6.3

davidmartos96 avatar Aug 22 '23 11:08 davidmartos96

firebase_core: 2.14.0 firebase_analytics: 10.4.3 firebase_crashlytics: 3.3.3 firebase_performance: 0.9.2+3 firebase_dynamic_links: 5.3.3 firebase_in_app_messaging: 0.7.3+3 firebase_remote_config: 4.2.3 firebase_messaging: 14.6.3 firebase_auth: 4.6.3

With the above set I'm gettin this error on Windows: firebase_core\windows\firebase_core_plugin.cpp(134,38): error C2039: 'GetApps': is not a member of 'firebase::App'

johannesvuorinen avatar Aug 23 '23 09:08 johannesvuorinen

@johannesvuorinen It's possible that the official Windows CPP implementation is being picked over the one from firebase_core_desktop.

You can try the following and call the function in your main before initializing Firebase:

import 'dart:io';

import 'package:firebase_core_desktop/firebase_core_desktop.dart';
import 'package:firebase_core_platform_interface/firebase_core_platform_interface.dart';

void forceUseFirebaseDesktopDartOnWindows() {
  if (Platform.isWindows) {
    FirebasePlatform.instance = FirebaseCore();
  }
}

The tests I did while developing were on Linux, but integration tests worked fine on Windows as well, so I'm not sure what could be the problem.

davidmartos96 avatar Aug 23 '23 10:08 davidmartos96

Got it working now. Thanks! We were using .apps directly from Firebase class and not from instance.

So now only firebase_auth lagging behind which would be separate PR.

johannesvuorinen avatar Aug 30 '23 09:08 johannesvuorinen

@johannesvuorinen Yes, firebase_auth 4.7.0 and greater migrated into using pigeon, so there are some incompatibilities with current Dart implementation in this repo. So yes, I agree a separate PR for that would be better once this gets merged and the CI includes the unit and integration tests

davidmartos96 avatar Aug 30 '23 11:08 davidmartos96

@davidmartos96 thanks for doing this work. Do you know if there's a good way to depend on your work instead of directly to the pub package? I've tried the following, but the naming is off and not as expected. Please advise.

 flutterfire_desktop_workspace:
    git:
      url: https://github.com/davidmartos96/flutterfire_desktop.git
      ref: fixes

JaredEzz avatar Oct 04 '23 17:10 JaredEzz

@JaredEzz Given that there are multiple packages involved transitively, it's a bit convoluted, but it can be done: In the dependencies overrides section:

dependency_overrides:
  firebase_auth_desktop:
    git:
      url: https://github.com/davidmartos96/flutterfire_desktop
      path: packages/firebase_auth/firebase_auth_desktop
      ref: fixes

  firebase_auth_dart:
    git:
      url: https://github.com/davidmartos96/flutterfire_desktop
      path: packages/firebase_auth/firebase_auth_dart
      ref: fixes

  firebase_core_desktop:
    git:
      url: https://github.com/davidmartos96/flutterfire_desktop
      path: packages/firebase_core/firebase_core_desktop
      ref: fixes

  firebase_core_dart:
    git:
      url: https://github.com/davidmartos96/flutterfire_desktop
      path: packages/firebase_core/firebase_core_dart
      ref: fixes

davidmartos96 avatar Oct 04 '23 20:10 davidmartos96

Perfect! I will try this out today - thanks so much for the reply

JaredEzz avatar Oct 04 '23 20:10 JaredEzz

Unfortunately, I was not able to use this for my project because we use TOTP Multi-factor authentication https://github.com/firebase/flutterfire/pull/11420 which requires firebase_auth 4.9.0 and I was greeted with this error message when adding the dependency overrides on your fixes branch. To be clear, I don't need desktop support for TOTP, that is handled for our customers on the web, but the lack of compatibility is a deal-breaker here.

Because every version of firebase_auth_desktop from git depends on firebase_auth >=4.0.0 <4.7.0 and [my_project] depends on firebase_auth ^4.9.0, firebase_auth_desktop from git is forbidden.
So, because [my_project]depends on firebase_auth_desktop from git, version solving failed.


You can try the following suggestion to make the pubspec resolve:
* Consider downgrading your constraint on firebase_auth: flutter pub add firebase_auth:^4.6.3

@davidmartos96 is there anything technical blocking supporting newer versions of firebase_auth?

JaredEzz avatar Oct 04 '23 21:10 JaredEzz

@JaredEzz I didn't update the code to firebase_auth >= 4.7.0 as it requires additional changes because it now uses Pigeon classes for the platform communication. Additionally it would make this PR much harder to review.

But ideally if this gets merged, the same e2e tests from the main flutterfire repo can be run against this desktop implementation and it would be easier to add the newest changes with higher confidence in other PRs.

davidmartos96 avatar Oct 04 '23 21:10 davidmartos96

Got it! Thanks for the explanation

JaredEzz avatar Oct 04 '23 21:10 JaredEzz

Any news on when this might be merged? Thanks for the PR!

vsutedjo avatar Oct 18 '23 12:10 vsutedjo

Any update on this?

NeoFahrenheit avatar Mar 05 '24 15:03 NeoFahrenheit