tiktok / tiktok-opensdk-ios

The TikTok OpenSDK features Login Kit and Share Kit which allow your users to log in using their TikTok account and share content from your app to TikTok.
https://developers.tiktok.com
Other
74 stars 29 forks source link

Migrate from CocoaPods to SPM #4

Closed nickdnk closed 1 year ago

nickdnk commented 1 year ago

Hello

This removes all need for CocoaPods and makes the package work with Swift Package Manager instead. Nobody should be creating new libraries/packages with CocoaPods at this time. The only reason to use CP at all is if a legacy package has not moved to SPM or if you have some odd CI flow you can't/won't update. As this is a new library with no dependencies, it should not be using CocoaPods.

Some work is still required to make all tests pass, and I'm not 100% sure about:

  1. Why there is localization when it doesn't appear to be used? I removed this as it doesn't appear to be necessary.
func localizedString(_ comment:String? = nil) -> String {
    guard let resPath = Bundle.TikTokBundle?.resourcePath else {
        return self
    }
    let identifier = Locale.current.TikTokSupportLangIdentifier()
    guard let langPath = Bundle(path: resPath)?.path(forResource: identifier, ofType: nil, inDirectory: "TikTokLocalizable") else {
        return self
    }
    let res = Bundle(path: langPath)?.localizedString(forKey: self, value: comment, table: "Passport")
    return res ?? self
}
  1. Why there are assets with arrows/logos that also don't appear to be used?

I changed localization to work via the built-in logic provided by XCode (see https://developer.apple.com/documentation/xcode/localizing-package-resources) and removed all CocoaPods-specific code. There is no reason to support CocoaPods when you have SPM, so this just makes it that much easier to maintain.

I removed the ObjC dummy/placeholder files as SPM does not support mixing languages for a single target. The package can still be used in ObjC projects. I have tested this as well, and all you need to do if using SPM is:

@import TikTokOpenAuthSDK; // (or Core/Share; the library you need)

In your header files where you want to use the library, i.e. AppDelegate.h;

To-do: CI/Travis needs to be adjusted to not run off CocoaPods. Example/Demo projects need to not use CocoaPods. All tests must be fixed/pass. I had to comment out one as the error code for it does not exist.

I've tested this and it installs/downloads and compiles correctly in my app. If anyone wants to use the SPM version while this is being reviewed, you can simply add my fork to your XCode project: https://github.com/nickdnk/tiktok-opensdk-ios

nickdnk commented 1 year ago
Screenshot 2023-07-04 at 01 27 44
nickdnk commented 1 year ago

@stephen-boyle This is unrelated to this issue, but can you maybe help figure out why the v2 OAuth flow for web doesn't work properly? Maybe ping someone else in the organisation. I've tried for a week to get it to exchange an authorisation code for an access token, but it keeps giving me The request parameters are malformed.. I emailed TikTok developer support about it but I got no response. There is also a StackOverflow thread with it here: https://stackoverflow.com/questions/76520793/tiktok-oauth-request-parameters-are-malformed. It's really frustrating. The exact same endpoint works fine if you include the code_verifier from the mobile SDK, which we use with this library. I just can't get it to work on web where code_verifier is not used.

The legacy endpoint works fine with the same parameters (excluding redirect_uri which isn't necessary there), but is being deprecated in September. I don't know what to do. We have a lot of different OAuth flows so I'm confident I'm doing it correctly. Only the TikTok one doesn't work.

nickdnk commented 1 year ago

Tested with ObjC project also. Seems to work. It builds at least and makes the methods available in .m-files, when importing with @import TikTokOpenAuthSDK; in header files.

nickdnk commented 1 year ago

The Share demo now works without CocoaPods. I removed the dependency on the photo library and made a simple implementation with the native PHPickerViewController instead. I also fixed a bunch of weird layout problems, removed storyboards and cleaned up some code. The project "works" but it doesn't share to TikTok because it just opens the open-api TikTok url and shows an nginx 404. The demo project requires iOS 14, which I imagine is fine as the library is still 11.0.

See https://github.com/tiktok/tiktok-opensdk-ios/pull/4/commits/f03d8a83a3cf927d38dee569cacca6524764d225 for details no this change.

Login demo still to be fixed.

nickdnk commented 1 year ago

Login demo now also works without pods. I also fixed some layout constraint errors in that and removed the storyboard and unnecessary wrapper views. I also added the new v2 scopes: user.info.profile and user.info.stats.

stephen-boyle commented 1 year ago

Hey @nickdnk , we were planning on adding SPM support soon, so thank you for taking the initiative to make this pull request. We are trying to set up a process to accept and review contributions so hang tight while we get that sorted out.

One thing I wanted to point out is that we want to support cocoapods for the applications that still use it. In the meantime, if you can re-include the podspecs into your PR and have them pass pod lib lint that would be much appreciated.

nickdnk commented 1 year ago

Hello

I've changed the structure of the files to support SPM, so I can't easily put CocoaPods back in, unfortunately. I don't use CocoaPods at all, so I have no real interest in making it work, and I already put a decent number of hours into this. If you want to support pods, you will have to implement it yourselves. If you do, I would suggest you merge this and then modify/add the pod spec files to fit the current SPM file structure and not the other way around. It shouldn't be too difficult. And the demo projects already work without pods; the package is linked locally in the XCode projects, so they just work without doing anything at all.

There is no real good reason (that I know of) to spend time on it though, as SPM is supported by all projects natively and can be mixed with CocoaPods anyway, so you won't be excluding anyone by not having pod support. They would just have to add the package via SPM and remove it from their Podfile; done.

The built-in localization of this probably won't work with CocoaPods either. What you had before was unnecessarily complicated. SPM localization is plug-and-play.

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

stephen-boyle commented 1 year ago

We really appreciate the hours you've spent working on this. We are in the progress of adding CocoaPods back in so that both options are available. We will merge your code once we have finished implementation and testing. Thanks again!

nickdnk commented 1 year ago

Alright. As mentioned before this PR also contains a lot of fixes and various cleanup, especially related to removal of storyboards and unnecessary wrapper views/containers and auto layout constraint errors. It also entirely removes the dependency on the third party image picker used in the share demo. In case you don't merge this PR and go the other way around, the fixes are worth going over and including.