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

chore: Update dependencies and fix tests #131

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

davidmartos96
Copy link

@davidmartos96 davidmartos96 commented Jul 30, 2023

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 fe794c8 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 😄

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 davidmartos96 changed the title Update dependencies and fix tests chore: Update dependencies and fix tests Jul 30, 2023
@dbebawy
Copy link

dbebawy commented Aug 10, 2023

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

@TimWhiting
Copy link
Collaborator

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.

@johannesvuorinen
Copy link

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

@davidmartos96
Copy link
Author

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

@johannesvuorinen
Copy link

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'

@davidmartos96
Copy link
Author

davidmartos96 commented Aug 23, 2023

@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.

@johannesvuorinen
Copy link

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.

@davidmartos96
Copy link
Author

@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

@JaredEzz
Copy link

JaredEzz commented Oct 4, 2023

@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

@davidmartos96
Copy link
Author

davidmartos96 commented Oct 4, 2023

@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

@JaredEzz
Copy link

JaredEzz commented Oct 4, 2023

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

@JaredEzz
Copy link

JaredEzz commented Oct 4, 2023

Unfortunately, I was not able to use this for my project because we use TOTP Multi-factor authentication firebase/flutterfire#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?

@davidmartos96
Copy link
Author

@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.

@JaredEzz
Copy link

JaredEzz commented Oct 4, 2023

Got it! Thanks for the explanation

@vsutedjo
Copy link

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

@NeoFahrenheit
Copy link

Any update on this?

@symtry
Copy link

symtry commented Jul 18, 2024

@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

Thanks for the excellent PR @davidmartos96. I had to add it as an override as pointed out above, but it resolved a nagging issue I've had with desktop builds causing "PlatformException(channel-error, Unable to establish connection on channel., null, null)" in Linux and Windows. The official packages have transitive dependencies that lock in most of the firebase libs a full one or two major version #s lower, and I suspected this was causing the platform specific issues. Pointing this out in case anyone else is deep in the 2nd google results page trying to fix this.

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.

8 participants