Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Separate ReceiptParser into SPM #2062

Closed
wants to merge 3 commits into from
Closed

Conversation

NachoSoto
Copy link
Contributor

@NachoSoto NachoSoto commented Nov 16, 2022

Fixes CSDK-17

Screenshot 2022-11-16 at 10 30 01

TODO:

- [ ] Create separate SPM for shared types (Logger?) This would be a pain to do with CocoaPods

@NachoSoto NachoSoto added WIP pr:feat A new feature labels Nov 16, 2022
@NachoSoto NachoSoto requested a review from a team November 16, 2022 18:31
@NachoSoto
Copy link
Contributor Author

Turns out this approach might not be supported: https://forums.swift.org/t/unable-to-integrate-a-remote-package-that-has-local-packages/53146/5

Might have to keep the folder structure the same, and instead expose a separate target in the main Package.swift.

We get the same result, but I was excited about this approach because it would allow creating many more smaller SPMs that would help us keep different parts of the SDK isolated like we do in Android.

@NachoSoto
Copy link
Contributor Author

I filed a Feedback for this:
Screenshot 2022-11-16 at 10 44 01

@NachoSoto NachoSoto force-pushed the receipt-parser-spm branch 4 times, most recently from 8a93941 to 5b6d5a8 Compare November 16, 2022 20:05
@NachoSoto
Copy link
Contributor Author

I'm gonna need a 3rd workaround for my 2nd workaround... Re-opening the project breaks it:
Screenshot 2022-11-16 at 12 39 07

@NachoSoto NachoSoto force-pushed the receipt-parser-spm branch 3 times, most recently from d8cb2e9 to 8966da4 Compare November 16, 2022 21:29
@NachoSoto NachoSoto force-pushed the receipt-parser-spm branch 5 times, most recently from 907e85b to 1dc251b Compare November 18, 2022 19:40
targets: resolveTargets()
targets: [
.target(name: "RevenueCat",
dependencies: [.byName(name: "ReceiptParser")],
Copy link
Member

Choose a reason for hiding this comment

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

In my experience, this will complicate things quite a bit.
It's a total headache to manage imports from a local package, we had something like this back in the objc days with PurchasesCoreSwift and it broke hybrids in all sorts of ways.

I think we should just have these two be independent packages, which just happen to have the same files.
If you use RC, you don't need the ReceiptParser package. If you use the ReceiptParser package, and you want to move to the full RevenueCat, you need to remove ReceiptParser and replace it with RevenueCat.

Copy link
Member

Choose a reason for hiding this comment

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

One bummer with this is that SPM from Xcode might prompt you to install both by default, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should just have these two be independent packages, which just happen to have the same files.

That's a great idea, which will simplify this tremendously.
I wanted to have a call with you to discuss the approach, but thanks for providing the best path forward already :D

Copy link
Member

Choose a reason for hiding this comment

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

happy to chat about it whenever!

@NachoSoto
Copy link
Contributor Author

New approach: #2096.

@NachoSoto NachoSoto closed this Nov 28, 2022
@NachoSoto NachoSoto deleted the receipt-parser-spm branch November 28, 2022 22:32
NachoSoto added a commit that referenced this pull request Nov 29, 2022
Fixes [CSDK-17].
New approach after #2062.
NachoSoto added a commit that referenced this pull request Dec 5, 2022
Fixes [CSDK-17].
New approach after #2062.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:feat A new feature WIP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants