Skip to content

Conversation

@dfabulich
Copy link
Contributor

@dfabulich dfabulich commented Oct 8, 2025

Fixes #49

This is marked as a draft PR, for three reasons.

  1. It doesn't build, and I don't know why. I'm importing SwiftUI, but it's failing to resolve any UI stuff.

    SkipFirebaseAnalytics+SwiftUI.kt:7:33 Unresolved reference 'saveable'.
    SkipFirebaseAnalytics+SwiftUI.kt:8:33 Unresolved reference 'saveable'.
    SkipFirebaseAnalytics+SwiftUI.kt:13:13 Unresolved reference 'ui'.
    SkipFirebaseAnalytics+SwiftUI.kt:17:41 Unresolved reference 'ViewModifier'.
    SkipFirebaseAnalytics+SwiftUI.kt:27:32 Unresolved reference 'Content'.
    SkipFirebaseAnalytics+SwiftUI.kt:27:42 Unresolved reference 'View'.
    SkipFirebaseAnalytics+SwiftUI.kt:29:24 Unresolved reference 'onAppear'.
    SkipFirebaseAnalytics+SwiftUI.kt:54:5 Unresolved reference 'View'.
    SkipFirebaseAnalytics+SwiftUI.kt:54:125 Unresolved reference 'View'.
    SkipFirebaseAnalytics+SwiftUI.kt:56:12 Unresolved reference 'modifier'.
    
  2. It's a stacked PR on top of Add Firebase Analytics constants #51. That PR should probably merge first.

  3. I'm a little unclear on what to do about the license. I copied this code wholesale from https://github.com/firebase/firebase-ios-sdk/blob/9cb17fca6368d49c2df4854f8e4099073395156d/FirebaseAnalytics/Sources/Analytics%2BSwiftUI.swift which is copyright Google under an Apache license. skip-firebase is licensed LGPL.

    My opinion is that the code in question is teeny tiny, and necessary for compatibility, so it should be legal to simply use it (and even relicense it) under fair use.

Skip Pull Request Checklist:

  • REQUIRED: I have signed the Contributor Agreement
  • REQUIRED: I have tested my change locally with swift test
  • OPTIONAL: I have tested my change on an iOS simulator or device
  • OPTIONAL: I have tested my change on an Android emulator or device

@cla-bot cla-bot bot added the cla-signed label Oct 8, 2025
@marcprux
Copy link
Contributor

marcprux commented Oct 8, 2025

  1. It doesn't build, and I don't know why. I'm importing SwiftUI, but it's failing to resolve any UI stuff.

I haven't looked closely at it, but the CI failure for the PR seems to be related to bridging of a type that has a "+" in the file name (a known problem, even though I haven't filed an issue for it yet).

I'm not sure if this is the same issue as you are seeing, but maybe try putting it in a file without a "+" character?

  1. It's a stacked PR on top of Add Firebase Analytics constants #51. That PR should probably merge first.

Merged!

  1. I'm a little unclear on what to do about the license.

Apache and (L)GPL are compatible, so there shouldn't be any issue with licensing. But I think that code is trivial enough to not need to copy over their license anyway.

@dfabulich dfabulich force-pushed the analytics-screen-modifier branch from 1db90f4 to d584c7b Compare October 8, 2025 05:49
@dfabulich dfabulich marked this pull request as ready for review October 8, 2025 05:51
@dfabulich
Copy link
Contributor Author

It was the +. The PR builds now and should be ready to merge.

@dfabulich
Copy link
Contributor Author

I don't reproduce this build failure on my machine when I run swift test.

@marcprux
Copy link
Contributor

marcprux commented Oct 8, 2025

The problem is coming from the skip android build part of the check, which tests building natively for the Android side (something that swift test doesn't do).

The error is the same issue as with the "+" in the filename, but this time with a "-":

 4 | private let Java_SkipFirebaseAnalytics-SwiftUIKt = try! JClass(name: "skip/firebase/analytics/SkipFirebaseAnalytics-SwiftUIKt")
   |                                       `- error: expected expression after unary operator

Maybe just call the file SkipFirebaseAnalyticsSwiftUI.swift 😄

@dfabulich dfabulich force-pushed the analytics-screen-modifier branch 2 times, most recently from 29c6240 to 9526f9d Compare October 8, 2025 18:40
@dfabulich dfabulich force-pushed the analytics-screen-modifier branch from 9526f9d to 47e9412 Compare October 8, 2025 18:52
@dfabulich
Copy link
Contributor Author

dfabulich commented Oct 8, 2025

I'm not 100% sure I fixed this build the "right way."

The build was failing because, in this PR, I've added code that depends on SwiftUI, providing our own View extension and ViewModifier. That works fine in Skip Lite mode, but doesn't work when I skip android build, because I didn't declare a dependency on skip-fuse-ui.

Other targets in this build depend on SkipUI, including SkipFirebaseAuth and SkipFirebaseMessaging, but they don't depend on SkipFuseUI, and they don't even import SwiftUI. Instead, SkipFirebaseAuth and SkipFirebaseMessaging use import skip.ui.__. I claim that there is no reason why they're doing this; I filed PR #53 to remove their unnecessary dependencies on SkipUI. (They would have failed the way my PR failed if they actually had used/needed it.) I now understand why they're depending on SkipUI, but I don't understand why they're directly importing skip.ui.__, or why they're not failing the build like mine did.

Anyway, I wasn't really able to rely on their example, and instead I added my dependency on SkipFuseUI in the SKIP_BRIDGE loop at the bottom of Package.swift.

I don't know if that's the right thing to do, but it seemed to work when I did it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bridge analyticsScreen API

2 participants