Skip to content

fix(firebase_crashlytics_tvos): port to firebase_crashlytics 5.3.0 (Swift/Pigeon), 0.0.2 - #28

Open
TheNoumanDev wants to merge 2 commits into
fluttertv:mainfrom
TheNoumanDev:fix/crashlytics-tvos-5.3.0
Open

TheNoumanDev wants to merge 2 commits into
fluttertv:mainfrom
TheNoumanDev:fix/crashlytics-tvos-5.3.0

Conversation

@TheNoumanDev

Copy link
Copy Markdown
Contributor

What does this PR do?

firebase_crashlytics 5.3.0 moved its Apple plugin to Swift + Pigeon. 0.0.1 still answers the old MethodChannel, so with 5.3.0 every call on tvOS fails with channel-error.

The native code is now upstream 5.3.0, with three tvOS edits (marked // tvOS:): import firebase_core_tvos, and allow os(tvOS) in two os(iOS) checks. pluginClass is now FirebaseCrashlyticsPlugin.

Package(s) touched: firebase_crashlytics_tvos

How was it tested?

  • Ran the package's example/ app
  • Verified on tvOS simulator (27.0): all calls work, and a forced crash() is detected on relaunch. With 0.0.1 they fail with channel-error.
  • Verified on a physical Apple TV
  • dart analyze is clean

…wift/Pigeon), 0.0.2

firebase_crashlytics 5.3.0 moved the Apple plugin from a MethodChannel
to Pigeon and from ObjC to Swift. 0.0.1 only answered the old channel,
so with 5.3.0 every call on tvOS failed with channel-error.

The native code is now upstream 5.3.0's Swift plugin, generated Pigeon
messages and ObjC helpers, with three tvOS edits marked `// tvOS:`:
import firebase_core_tvos, and allow tvOS in two os(iOS) checks.
pluginClass is now FirebaseCrashlyticsPlugin.

Verified on the tvOS 27.0 simulator: every Crashlytics call returns
without error, and a forced crash is reported on the next launch.
@TheNoumanDev

Copy link
Copy Markdown
Contributor Author

@DenisovAV @MAUstaoglu Please take a look, thanks

@DenisovAV DenisovAV left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approve. The port is faithful where it says it is, and the premise holds.

The port is verbatim

Every ported file diffed against firebase_crashlytics 5.3.0's ios/firebase_crashlytics/Sources/:

  • Constants.swift, CrashlyticsPlatformHelpers.m, include/CrashlyticsPlatformHelpers.h, include/Crashlytics_Platform.h, include/ExceptionModel_Platform.h — byte-identical. The two lines dropped from Crashlytics_Platform.h move it to upstream, not away from it.
  • FirebaseCrashlyticsMessages.g.swift — one hunk, the #if the report names.
  • FirebaseCrashlyticsPlugin.swift — two hunks, the two the report names.

Three edits claimed, three found, nothing else in 930 added lines.

The channel-error premise

5.3.0's Dart side talks on ten channels only, all dev.flutter.pigeon.firebase_crashlytics_platform_interface.FirebaseCrashlyticsHostApi.*, and the ten the new .g.swift registers are that same set exactly. plugins.flutter.io/firebase_crashlytics — the one channel 0.0.1 answered — appears nowhere in the platform interface's call path.

This is live on pub rather than hypothetical: published 0.0.1 declares firebase_crashlytics: ^5.2.4, which resolves to 5.4.0 today, so every fresh install of 0.0.1 is already in that state.

The chain a rename could have broken

FirebaseCrashlytics._ passes 'plugins.flutter.io/firebase_crashlytics' to FirebasePlugin; the platform interface asserts pluginConstants['isCrashlyticsCollectionEnabled'] != null; firebase_core_tvos's registry keys that map by each plugin's flutterChannelName. The Swift still returns that exact string and still implements pluginConstants(for:), so the startup assert is satisfied.

The Swift↔ObjC names work out against the tvOS core specifically: firebase_core_tvos declares both @protocol FLTFirebasePlugin and @interface FLTFirebasePlugin, so Swift sees the protocol as FLTFirebasePluginProtocol; registerFirebasePlugin: arrives as register(_:), pluginConstantsForFIRApp: as pluginConstants(for:). All five @required members are implemented.

Pods, versions, resolution

Firebase/Crashlytics ~> 12.15.0 agrees with firebase_core_tvos's Firebase/CoreOnly ~> 12.15.0, and the report's "upstream uses 12.18.0" matches firebase_core 4.14.0's firebase_sdk_version.rb. Dropping the LIBRARY_NAME/LIBRARY_VERSION defines is right — nothing reads them now. pubspec 0.0.2 = podspec 0.0.2 = ## 0.0.2.

The README's platform-interface sentence names a narrower pairing than the dependency requires, but is safe either way: firebase_core_platform_interface 8.1.0 and 8.1.1 generate byte-identical Pigeon Dart, as do firebase_crashlytics_platform_interface 3.9.0 and 3.9.1, so firebase_core_tvos 0.0.3 (regenerated for 8.1.0) is valid on the 8.1.1 the example resolves.

Locally: flutter pub get resolves for the package and the example, dart analyze is clean on both, the test passes.

Findings — none blocking

1. ^5.3.0 is the right bound; worth recording why. The package resolves firebase_crashlytics 5.4.0 today, not 5.3.0, and that is safe: 5.3.0, 5.4.0 and 6.0.0 ship byte-identical ios/.../Sources apart from versionNumber, and all three pin firebase_crashlytics_platform_interface: ^3.9.0. 6.0.0 is excluded by the caret and would have to be — it requires firebase_core: ^5.0.0, which firebase_core_tvos 0.0.3 (firebase_core: ^4.13.0) cannot satisfy. A pubspec comment saying the window is verified rather than assumed would stop the next reader tightening it needlessly or widening it on the next major.

2. The whole class has a gate that runs on Linux. What broke here is a set mismatch between the dev.flutter.pigeon.* names in tvos/Classes/*.g.swift and those in the resolved platform interface's lib/src/pigeon/messages.pigeon.dart. After pub get both are text in the tree, so comparing the two sets is a Dart test or a sixth check_repo.py rule, and it would have failed 0.0.1 against 5.3.0 with no Apple hardware involved. R1–R5 read no native sources today: R5 checks that a pluginClass is declared, not that a type by that name exists under tvos/ — the other half of this PR's rename, and the same kind of string comparison. This family has paid for the gap three times now (firebase_core_tvos 0.0.2's 15-vs-14-element payload, firebase_app_check_tvos #26, and this).

3. The green check on this package is thinner than it looks. test/firebase_crashlytics_tvos_test.dart is still the generated expect(1 + 1, 2), so the job proves resolution and analysis only — by design, per the workflow header, which is what makes the PR's testing box the only evidence for the native half.

4. A device run is the one claim 0.0.2 cannot make and 0.0.1 could. The README table is honest about it. For a crash reporter it is also the claim that matters most: the simulator exercises the Pigeon surface, while dSYM handling and the signal/NSException path on real hardware are what the package exists for. Worth closing before publishing 0.0.2 to pub.

5. versionNumber will not track the resolved package. It is what the native side reports through FIRApp registerLibrary:withVersion:, so an app on 5.4.0 reports 5.3.0. Cohort-consistent, and already drifted in a sibling: firebase_analytics_tvos 0.0.1 ships versionNumber = "11.4.6" against firebase_analytics: ^12.4.3.

6. The Firebase library identity changes and the changelog doesn't say so. 0.0.1 reported flutter-fire-cls-tvos / 0.0.1 from the podspec defines; 0.0.2 reports upstream's flutter-fire-cls / 5.3.0. That matches the five already-migrated siblings — fn, analytics, appcheck, perf, rc all dropped the suffix — so it is the right call, but the four still-ObjC packages (core, auth, fst, fcm) keep -tvos, so what the fork tells Firebase is now split down the middle. One changelog line would record the decision rather than leave it to be rediscovered.

Not this PR

CI is red on sqflite_tvos, which fails in pub get because the workflow pins FLUTTER_VERSION: 3.44.0 while that package needs 3.47.7; the ci job only mirrors the matrix.

The larger thing this port surfaces: firebase_crashlytics 6.0.0 and firebase_core 5.0.0 both landed on 15 October, five days before this PR, and 6.0.0 requires the 5.x core train. Nothing here is affected, but the tvOS family is now a major behind at the Dart level, and the gate on catching up is firebase_core_tvos against core 5.0.0 — which pins Firebase Apple SDK 12.19.0 where the tvOS pods pin 12.15.0.

@TheNoumanDev

Copy link
Copy Markdown
Contributor Author

Hey @DenisovAV Thanks for the detailed review!

Added in 3718029:

  • (1) A pubspec comment on why ^5.3.0: 5.4.0 has the same native sources apart from versionNumber, and 6.0.0 needs firebase_core 5.x.
  • (6) A CHANGELOG line for the library name change (flutter-fire-cls-tvos → flutter-fire-cls).

(4) Tested on a real device: Apple TV 4K (3rd gen), tvOS 27.0, release build of the example. The app starts, and the test crash goes through the NSException path (FirebaseCrashlyticsTestCrash). After relaunch, the report shows up in the Firebase console.

(2) and (3) seem better as separate repo-wide changes, so I've left them out of this PR. (5) stays as is, the same as the other ports.

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.

2 participants