Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[in_app_purchase] Define clang module for iOS #2125

Merged
jmagman merged 3 commits into from
Oct 1, 2019
Merged

[in_app_purchase] Define clang module for iOS #2125

jmagman merged 3 commits into from
Oct 1, 2019

Conversation

@jmagman
Copy link
Member

@jmagman jmagman commented Sep 30, 2019 •
edited
Loading

Description

  • Limit the supported podspec platform to iOS so tests don't run (and fail) for macOS.
  • Define the module by setting DEFINES_MODULE in the podspec. See CocoaPod modular headers docs.
  • Explicitly set VALID_ARCHS to architectures included in the Flutter universal binary. This is the CocoaPods-suggested workaround to prevent tests from running on the i386 simulator. See Fix linting when armv7 is included but i386 isn't CocoaPods/CocoaPods#8159.
  • Fix PaymentQueueTest.m. It doesn't compile and it wasn't actually included in the example Xcode project.
  • Fix a few analyzer warnings:
    • nullability warning in FIAPReceiptManager
    • NSNumber warning to check against nil or -boolValue

Related Issues

Fixes flutter/flutter#41648.
See also flutter/flutter#41007.

Tests

  • Add a test_spec to the podspec so the CocoaPods linter will test them. These tests fail before the s.platform and s.pod_target_xcconfig changes.
  • Move existing tests to new Tests directory so they are run on pre-submit!
  • Fix availability in tests so they pass on older simulators.

Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See [Contributor Guide]).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the [Flutter Style Guide].
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy].
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the [CLA].
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@jmagman jmagman requested review from cyanglaz and mklim September 30, 2019 21:40
@jmagman jmagman changed the title [in_app_purchase] Define clang module for iOS WIP [in_app_purchase] Define clang module for iOS Sep 30, 2019
@jmagman jmagman changed the title WIP [in_app_purchase] Define clang module for iOS [in_app_purchase] Define clang module for iOS Sep 30, 2019
jmagman
jmagman commented Sep 30, 2019
View reviewed changes
packages/in_app_purchase/ios/Tests/Stubs.h
#import "InAppPurchasePlugin.h"

NS_ASSUME_NONNULL_BEGIN
API_AVAILABLE(ios(11.2), macos(10.13.2))
Copy link
Member Author

@jmagman jmagman Sep 30, 2019

Choose a reason for hiding this comment

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

Added availability.

jmagman
jmagman commented Sep 30, 2019
View reviewed changes
packages/in_app_purchase/ios/Tests/TranslatorTest.m
@"localizedTitle" : @"title",
@"localizedDescription" : @"des",
}];
if (@available(iOS 11.2, *)) {
Copy link
Member Author

@jmagman jmagman Sep 30, 2019

Choose a reason for hiding this comment

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

Added availability so these tests passed with older SDKs.

jmagman
jmagman commented Sep 30, 2019
View reviewed changes
packages/in_app_purchase/ios/Tests/InAppPurchasePluginTest.m
@@ -0,0 +1,206 @@
// Copyright 2019 The Chromium Authors. All rights reserved.
Copy link
Member Author

@jmagman jmagman Sep 30, 2019

Choose a reason for hiding this comment

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

These test files were moved from the example/ project to the new Tests/ directory. Added symlinks from the old location so the tests still pass in the example app.

Copy link
Member Author

@jmagman jmagman Sep 30, 2019

Choose a reason for hiding this comment

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

Turns out Xcode really didn't link the symlinking. I changed the Xcode project to just point at the new location instead of following the symlinks.

jmagman
jmagman commented Sep 30, 2019
View reviewed changes
packages/in_app_purchase/example/ios/in_app_purchase_pluginTests/PaymentQueueTest.m
@@ -1,183 +0,0 @@
// Copyright 2019 The Chromium Authors. All rights reserved.
Copy link
Member Author

@jmagman jmagman Sep 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@cyanglaz This test doesn't compile and wasn't included in the Xcode project, so I deleted it. I can file an issue for it to be re-added and fixed to pass, if you still find value in it.

Copy link
Contributor

@cyanglaz cyanglaz Sep 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, please add this test case back to the project and remove the .testing part to fix the compilation. I am not sure why it would be like this in the first place.

Copy link
Member Author

@jmagman jmagman Sep 30, 2019

Choose a reason for hiding this comment

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

Done.

mklim
mklim reviewed Sep 30, 2019
View reviewed changes
Copy link
Contributor

@mklim mklim left a comment

Choose a reason for hiding this comment

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

LGTM, but @cyanglaz should also review and approve.

cyanglaz
cyanglaz approved these changes Sep 30, 2019
View reviewed changes
Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

LGTM overall. Left some comments.

packages/in_app_purchase/ios/Classes/InAppPurchasePlugin.m
payment.applicationUsername = [paymentMap objectForKey:@"applicationUsername"];
NSNumber *quantity = [paymentMap objectForKey:@"quantity"];
payment.quantity = quantity ? quantity.integerValue : 1;
payment.quantity = (quantity != nil) ? quantity.integerValue : 1;
Copy link
Contributor

@cyanglaz cyanglaz Sep 30, 2019

Choose a reason for hiding this comment

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

what is the different or benefit we gain with this change?

Copy link
Member Author

@jmagman jmagman Sep 30, 2019

Choose a reason for hiding this comment

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

It fixes a static analyzer warning:

    157:24: warning: Converting a pointer value of type 'NSNumber *' to a primitive boolean value; instead, either compare the pointer to nil or call -boolValue
        payment.quantity = quantity ? quantity.integerValue : 1;

packages/in_app_purchase/ios/Tests/Stubs.h Outdated
@interface FIAPReceiptManagerStub : FIAPReceiptManager
@end

API_AVAILABLE(ios(7.0), macos(10.9))
Copy link
Contributor

@cyanglaz cyanglaz Sep 30, 2019

Choose a reason for hiding this comment

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

Flutter supports 8 and up so this might not be necessary. I'm ok leave it though.

Copy link
Member Author

@jmagman jmagman Sep 30, 2019

Choose a reason for hiding this comment

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

I'll remove it, most of the others should also have API_AVAILABLE(ios(7.0), macos(10.9)) so this isn't consistent.

packages/in_app_purchase/pubspec.yaml Outdated
author: Flutter Team <[email protected]>
homepage: https://github.com/flutter/plugins/tree/master/packages/in_app_purchase
version: 0.2.1+4
version: 0.2.2
Copy link
Contributor

@cyanglaz cyanglaz Sep 30, 2019

Choose a reason for hiding this comment

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

If there is no public API change, we just need to update the patch, thus 0.2.1+5
I might have missed something though. Please point it out if so.

Copy link
Member Author

@jmagman jmagman Sep 30, 2019

Choose a reason for hiding this comment

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

The only public API change was adding the nullability return value. The podspec change means this will be imported as a module, so modulemaps get generated, and CocoaPods does a lot of other magic. I'm happy to change this to 0.2.1+5 though!

@jmagman jmagman mentioned this pull request Sep 30, 2019
Closed
13 tasks
@jmagman jmagman merged commit 6070d19 into flutter:master Oct 1, 2019
@jmagman jmagman deleted the iap-module branch October 1, 2019 01:25
mormih pushed a commit to mormih/plugins that referenced this pull request Nov 17, 2019
@jmagman @mormih
[in_app_purchase] Define clang module for iOS (flutter#2125)
7fe85bb
sungmin-park pushed a commit to sungmin-park/flutter-plugins that referenced this pull request Dec 17, 2019
@jmagman
[in_app_purchase] Define clang module for iOS (flutter#2125)
9827291
@jmagman jmagman mentioned this pull request Sep 13, 2022
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@mklim mklim mklim left review comments

@cyanglaz cyanglaz cyanglaz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[in_app_purchase] Define clang modules plugin for iOS

4 participants

@jmagman @cyanglaz @mklim @googlebot