Skip to content

Conversation

@ncooke3
Copy link
Member

@ncooke3 ncooke3 commented Mar 25, 2021

FIAM was not working on SPM because the resource bundle was not generating.

There was a lot of experimentation here so I wanted to make note of some realizations/learnings:
• In order for SPM to generate a bundle for a target, the target's resources need to be in the target's Sources directory.
• The Package manifest will attempt to compile and build all targets (an issue for targets with platform-specific resources).
• FIAM only provides default UI implementations for iOS (so I've removed the blank tvOS storyboard)
• SPM generates target bundles in a single directory. So if target B is listed as a dependency of target A and both targets defines resources in their declarations, then SPM will generate a Target_B.bundle and a Target_A.bundle at the same directory level (this was against my intuition as I thought target B's bundle would be within target A's bundle since A depends on B).

Note on SPM & platform specific resources:
• There currently isn't a clear cut way to define platform specific resources. See the conversation on the Swift forums here. The workaround here is to do something similar to what was done in SPM for Analytics by creating a new target containing FIAM's iOS resources that is conditionally depended on in the primary FIAM target.

Future work:
Translating FIAM's default UI implementations from storyboards to programmatic UIKit should make this type of problem easier to fix in the future because we can use preprocessor directives (which we can't use in storyboards). Plus, programmatic UI's should be easier to maintain and update.

Fixes #7715.

@google-cla google-cla bot added the cla: yes label Mar 25, 2021
@ncooke3 ncooke3 requested review from christibbs and paulb777 March 25, 2021 18:20
@google-oss-bot google-oss-bot added the api: inappmessaging Firebase In App Messaging label Mar 25, 2021
@google-oss-bot
Copy link

google-oss-bot commented Mar 25, 2021

Coverage Report

Affected SDKs

  • FirebaseInAppMessaging-iOS-FirebaseInAppMessaging.framework

    SDK overall coverage changed from 43.78% (7fb172b) to 43.92% (8648a9c) by +0.14%.

    Filename Base (7fb172b) Head (8648a9c) Diff
    FIRIAMDefaultDisplayImpl.m 1.60% 1.56% -0.04%

Test Logs

@maksymmalyhin
Copy link
Contributor

@ncooke3 One option that seems to be worth trying if other approaches fail:

  • define separate targets with resources per platform, e.g. FirebaseInAppMessagingResources_ios, FirebaseInAppMessagingResources_tvos, etc.
  • define the resource targets as conditional dependencies to FirebaseInAppMessagingTarget e.g. like here:
    condition: .when(platforms: [.iOS]))],

@christibbs
Copy link
Contributor

I see 🟢

I'll run tests on this today and approve if we're looking good.

Copy link
Member

@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

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

Nice way to eliminate complexity from the problem!

@christibbs
Copy link
Contributor

christibbs commented Apr 5, 2021

We're looking good. FIAM still works on iOS and tvOS via CocoaPods. FIAM iOS works via Swift Package Manager, not for tvOS though (SPM). @ncooke3 this is expected, right?

@maksymmalyhin
Copy link
Contributor

@ncooke3 Great investigation, thank you for doing it!

I may be worth creating a doc like this for the platform-specific resource in SPM with a summary, conclusions and recommendations for teams that use or plan to such resources. I can be done as a follow up for the PR.

@christibbs
Copy link
Contributor

Tested this on SPM and CocoaPods for iOS and tvOS. Everything's working.

Comment on lines -20 to -21
#import <Foundation/Foundation.h>

Copy link
Member Author

Choose a reason for hiding this comment

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

This is already imported in the header file.

#import "FirebaseInAppMessaging/Sources/DefaultUI/Modal/FIRIAMModalViewController.h"
#import "FirebaseInAppMessaging/Sources/Private/Util/FIRIAMTimeFetcher.h"
#import "FirebaseInAppMessaging/Sources/Public/FirebaseInAppMessaging/FIRInAppMessaging.h"
#import "FirebaseInAppMessaging/Sources/Public/FirebaseInAppMessaging/FIRInAppMessagingRendering.h"
Copy link
Member Author

Choose a reason for hiding this comment

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

Another duplicate import.

Copy link
Contributor

@maksymmalyhin maksymmalyhin left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ncooke3 ncooke3 merged commit a05ce1c into master Apr 6, 2021
@ncooke3 ncooke3 deleted the nc-fiam-spm-fix branch April 6, 2021 21:09
@firebase firebase locked and limited conversation to collaborators May 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

api: inappmessaging Firebase In App Messaging cla: yes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

In-App Messaging via SPM missing resource bundle

5 participants