microsoft / appcenter-sdk-apple

Development repository for the App Center SDK for iOS, macOS and tvOS.
Other
565 stars 224 forks source link

add support to override default bundle for resources #2524

Closed ilendemli closed 8 months ago

ilendemli commented 9 months ago

Things to consider before you submit the PR:

Description

Adds a nullable property which can be used to override the default bundle for Distribute to search for its resources.

Related PRs or issues

https://github.com/microsoft/appcenter-sdk-apple/issues/2297

Misc

This is a workaround to use binary swift packages to improve build times. Each xcframework has to be zipped and uploaded separately. Example swift package release can be found here https://github.com/ilendemli/appcenter-sdk-apple-spm/releases/tag/5.0.4

ilendemli commented 9 months ago

@microsoft-github-policy-service agree

ilendemli commented 9 months ago

I am also creating a workflow, to automatically separate the XCFrameworks from this repository and to release it on GitHub. You can feel free to fork that repository too after this has been merged, so it is official: https://github.com/ilendemli/appcenter-sdk-apple-spm/tree/workflow

MikhailSuendukov commented 8 months ago

Hi @ilendemli Hello and thank you for your contribution. Could you please describe in more detail how your pull request fixes the problem that was described in the issue and what is the use case for this feature?

ilendemli commented 8 months ago

@MikhailSuendukov So the underlying issue is, that, when building xcframeworks from the repo SWIFTPM_MODULE_BUNDLE is not defined. Currently, the bundle named APP_CENTER_DISTRIBUTE_BUNDLE_NAME inside the bundle where the MSACDistribute class is, will be used as the bundle for its resources. When bundling the xcframeworks as a swift package, we have to include the resources bundle as well. Getting the bundle by using MSACDistribute class won't work here. This PR adds the ability to set the bundle to a custom one for Distribute to use, so the alert dialogs are localized properly.

EDIT: if you want to compare this, clone my repo from here https://github.com/ilendemli/appcenter-sdk-apple-spm, download the xcframeworks.zip from this repo, unzip it and move the xcframeworks to the appcenter-sdk-apple-spm directory. Edit Packages.swift to use path for the binaryTargets and target them to the xcframeworks. Use the package inside an example app and setup Distribute as usual to trigger an update or error dialog (for example using wrong secret). The alert is not localized.

Then clone this PR, build Distribute, get AppCenterDistribute.xcframework and replace the one inside appcenter-sdk-apple-spm with the newly built one. Add

if let bundleURL = Bundle.main.url(forResource: "AppCenter_AppCenterDistributeTarget", withExtension: "bundle") {
    Distribute.resourceBundle = Bundle(url: bundleURL)
}

to your app, and trigger the alert again. Alert should be localized.

MikhailSuendukov commented 8 months ago

Thank you very much for the information, but could you please clarify which application you mean?

ilendemli commented 8 months ago

I meant for you to create a new app project, and to integrate AppCenter SDK using a precompiled binary swift package.

EDIT: Before After
IMG_1805 IMG_1806
MikhailSuendukov commented 8 months ago

The fact that an additional repository is used to solve this is a problem. Is it possible to fix the problem within our repository only?

ilendemli commented 8 months ago

Why is an additional repository a problem? That repository will maintain itself automatically with GitHub actions and users will have the option to use appcenter either from code or from binary.

EDIT: Also that way users of appcenter only have to update the url to the repository. It won‘t require them to change anything. It might require additional work for the end user if we try to integrate it into this repository. airbnb does the same for Lottie with lottie-spm

DmitriyKirakosyan commented 8 months ago

@ilendemli While we appreciate the effort you've put into implementing this feature, to ensure proper functionality, the integration needs to be conducted directly from this repository. As such, using an alternative integration method, like adding SPM through your repository's link, remains unsupported. This, in turn, means that the API introduced in this pull request will also be unsupported. Consequently, introducing an API that is known to be unsupported from the outset may not be the best course of action.

If you still require this feature, I would suggest creating a fork and using that for your integration purposes.

ilendemli commented 8 months ago

Interesting take. Maybe I did not express myself clearly but anyway. The API changes does not affect any functionality but opens up possibilities. But if this is not wanted, then I have to respect that decision.

lucen-ms commented 8 months ago

@ilendemli thanks for your effort but we'd like to have a complete solution in our repository (not depending on other repositories as @MikhailSuendukov wrote). If you can update this PR to implement everything within our repository, we will be happy to merge it.

ilendemli commented 8 months ago

Someone else might come up with a better solution. For now, I have updated my repository to pull this repo, apply a patch, compile and bundle a binary swift package. You might still want to take the translation fixes I made here though.

EDIT: Also I think you misunderstood the way this would have worked out. This repo would not be depending on the other repo but the other way around. You would not have to maintain the other repo, as it would have maintained itself automatically. Whenever there was a new release here, the other repo would bundle a binary swift package for people to use there.