Customer Center compilation flag #4149
Conversation
Generated by 🚫 Danger |
| @@ -6742,6 +6742,7 @@ | |||
| ONLY_ACTIVE_ARCH = YES; | |||
| SDKROOT = iphoneos; | |||
| SUPPORTS_MACCATALYST = YES; | |||
| SWIFT_ACTIVE_COMPILATION_CONDITIONS = CUSTOMER_CENTER_ENABLED; | |||
There was a problem hiding this comment.
in order to merge to main, we just make a new PR that kills this line. And then we bring it back in the integration branch
There was a problem hiding this comment.
Note that since the flag is only defined for the Xcode project, SPM and CocoaPods builds will already evaluate it to false and skip Customer Center. Carthage won't, however, since it reads from Xcode, so we should remove it to merge into main
| #if CUSTOMER_CENTER_ENABLED | ||
|
|
||
| import Foundation |
There was a problem hiding this comment.
okay, I'll admit that it's annoying to have this in 30 files, but it legitimately took me just a couple of mins to add
There was a problem hiding this comment.
and we'll only need to remove it when we're ready to ship the feature
There was a problem hiding this comment.
I bet we can probably leave it out in some of these files... But better to be sure 👍
There was a problem hiding this comment.
No idea if this is in the realm of possibilities, but would it be possible to have the the flag only around public entry points of the Customer Center feature? E.g. Purchases.loadCustomerCenter(), CustomerCenterView() and the presentCustomerCenter() modifier?
There was a problem hiding this comment.
While that should be fine for RevenueCat (we would just need the flag for loadCustomerCenter and CustomerCenterConfigData), since CustomerCenterConfigData is used throughout RevenueCatUI, we would need to wrap many files there.
There was a problem hiding this comment.
What if.. we only wrap the constructor of CustomerCenterConfigData (or make the constructor public if the flag exists, internal otherwise). We'd be exposing some extra types, but no dev would be able to actually do anything with them because they can't construct anything.
(Just spitballing, lmk if this is dumb.)
There was a problem hiding this comment.
There are a bunch of internal types inside CustomerCenterConfigData so we would need to wrap a bunch of constructors... It would however, remove the need to wrap the code in RevenueCatUI...
Having said that, we would still be exposing new types which can potentially change, even if they are unusable... So I don't think it's ideal 😓
There was a problem hiding this comment.
Hmm yea, not ideal indeed. 🤔
There was a problem hiding this comment.
Yeah I figured we can probably fine tune this a bit. TBH I figured I'd go with the most brainless imaginable approach which is to wrap all the things just because:
- I'm not that familiar with the implementation so the dumb approach took me 10 mins
- It's so easy that removing it would also just take 10 mins of brainless work when we're done with it
- It's all temporary and we hope to kill this stuff in like maybe a month
If you feel that it's still worth the trouble, I'm more than happy to do it, but it might take me a while, so feel free to take it over and carry it through the finish line.
I figured my biggest contribution here would be setting up the skeleton, i.e.: the compilation flag and the usage, feel free to adapt it to whatever is best!
There was a problem hiding this comment.
Yeah I think you're making very good points. Not worth the extra effort to do fine tuning imo. Also, we don't have a good idea what fine tuning would even look like yet.
|
If you wanna test out the branch, I'd recommend running things as usual once, then removing the flag from the project settings (but keeping it in the code), then running again to verify that that actually removes all of CustomerCenter. You might need to clean builds if Xcode doesn't realize that the flag affects more than one project automatically. |
| #if CUSTOMER_CENTER_ENABLED | ||
|
|
||
| import Foundation |
There was a problem hiding this comment.
I bet we can probably leave it out in some of these files... But better to be sure 👍
tonidero
merged commit 92db864
into
integration/customer_support_workflow
We have a long-running integration branch for Customer Center, long running integration branches usually get very tricky to merge as time passes on.
This PR attempts to solve it by creating a single custom compiler flag that enables the Customer Center feature.
Here is what the compilation flag looks like:
Then all we need to do to integrate into main is to delete that flag. And re-add it for development. Easy peasy.
And when we want to ship Customer Center for real, we just delete all usages of that flag.
If the flag isn't defined, then the code evaluates to false and is simply skipped by the compiler. If it evaluates to true, then the code runs.