-
Notifications
You must be signed in to change notification settings - Fork 2
Potential fix for iOS release build mode #17
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
base: main
Are you sure you want to change the base?
Conversation
Thanks for catching this. We had a similar problem on the react-native plugin, and the solution was to simply remove the I'll defer to my colleagues for the rest of this PR (@brandonjenniges, @tamerbader), and we'll need to publish the new version anyway. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Checking in about what we expect to happen with this; as Tobi noted, you shouldn't need DEBUG to use the MockReader, so I'm inclined to reject the PR... but that means that something else made lemz90 think it was debug-only. That's false, or should be. So what made it seem true?? |
e264c3d
to
40810c7
Compare
Sorry just circling back to this. I assumed it was Debug only since the podspec had it specified as so. I probably should have made an issue first to identify the correct fix, sorry for the extra complication. I've updated my branch to remove the I've also confirmed that the example app builds in release mode and debug mode. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, thank you for updating! This aligns with what we do for React Native as well: https://github.com/square/mobile-payments-sdk-react-native/blob/main/mobile-payments-sdk-react-native.podspec#L18
cc @tamerbader @brandonjenniges for releasing, since I know y'all are working on an MPSDK update anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably bump s.version in Podspec as well (patch is fine?).
Problem
Our app, as well as the example app, fails to build on iOS in release mode due to
MockReaderUI
being unavailable. This is particularly relevant for Flutter developers who want to use the plugin without manually managing iOS dependencies -- although I was never able to get it to build by addingpod MockReaderUI
to the app's Podfile.Solution
Added conditional compilation for
MockReaderUI
related code using#if DEBUG
to ensure the plugin builds successfully in both debug and release modes. This is a common pattern in iOS development for handling debug-only features.The solution doesn't require any additional pod or SPM dependencies in the app's Podfile.
Testing
Note
While this solution resolves the immediate build issue, I'm open to feedback if there's a more idiomatic approach that the Square team would prefer. Possibly something more on the flutter plugin or cocoapods side of things.
I also haven't tested that MockReader works in flutter debug mode but I assume it would. I thought I'd go ahead and put this up since it got us building release mode. Also wanted to add, this isn't critical for us since we can use my fork in the pubspec.yaml.