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

[in_app_purchase_storekit] Add support for macOS #6517

Merged
merged 23 commits into from
Nov 30, 2022

Conversation

IVLIVS-III
Copy link
Contributor

@IVLIVS-III IVLIVS-III commented Sep 29, 2022

This PR adds MacOS as a supported platform to in_app_purchase_storekit.

The implementation is now fully shared between iOS and MacOS (no code duplication), at least as far as permitted by the underlying storekit framework.
On MacOS the following methods are no-ops:

  • showPriceConsentIfNeeded
  • presentCodeRedemptionSheet

Part of flutter/flutter#84391.

I tried my best to follow the steps outlined in this comment on the original issue.
If there is anything missing, I'm happy to update the PR.

I intend to submit the necessary changes to the app-facing plugin in_app_purchase in a second PR (#6519).

Note: I am aware of two previous PRs (#2708 and #5854). But those are either closed or still in draft and have not had any recent activity.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@IVLIVS-III IVLIVS-III changed the title [in_app_purchase] Add support for MacOS [in_app_purchase_storekit] Add support for MacOS Sep 29, 2022
@IVLIVS-III
Copy link
Contributor Author

IVLIVS-III commented Sep 29, 2022

Addressing the 4 failing tests:

  1. publishable
    I expect this test to pass once relative path dependencies have been reverted.

  2. macos-platform_tests CHANNEL:master
    I don't know why no driver tests are found for in_app_purchase/in_app_purchase_storekit/example.

  3. macos-build_all_plugins CHANNEL:stable
    I expect this test to pass if the all_plugins project had a minimum deployment target of 10.15 for macOS. But I don't know how to change this.

  4. macos-build_all_plugins CHANNEL:master
    I expect this test to pass if the all_plugins project had a minimum deployment target of 10.15 for macOS. But I don't know how to change this.

@IVLIVS-III IVLIVS-III marked this pull request as ready for review September 29, 2022 23:36
@a-wallen
Copy link

a-wallen commented Oct 5, 2022

Could you delete the assets (*png) from the example?

@stuartmorgan stuartmorgan self-requested a review October 6, 2022 01:53
Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

I haven't had time to do a full review yet, but to address the above:

  1. macos-platform_tests CHANNEL:master
    I don't know why no driver tests are found for in_app_purchase/in_app_purchase_storekit/example.

It looks like it was set up wrong when the current (placeholder, unfortunately) tests were added, and since there's an exclusion still for iOS from before it was added, that wasn't caught. Moving example/integration_test/test/integration_test.dart to example/integration_test/integration_test.dart should fix that. You should also be able to remove the exclusion from script/configs/exclude_integration_ios.yaml when fixing that.

  1. macos-build_all_plugins CHANNEL:stable
    I expect this test to pass if the all_plugins project had a minimum deployment target of 10.15 for macOS. But I don't know how to change this.

This would be done in script/tool/lib/src/create_all_plugins_app_command.dart. See _updateAppGradle for an example of making a similar change to some Android values.

@IVLIVS-III IVLIVS-III changed the title [in_app_purchase_storekit] Add support for MacOS [in_app_purchase_storekit] Add support for macOS Oct 7, 2022
@IVLIVS-III
Copy link
Contributor Author

@stuartmorgan

This would be done in script/tool/lib/src/create_all_plugins_app_command.dart. See _updateAppGradle for an example of making a similar change to some Android values.

I tried implementing it like outlined but unfortunately I hit a road-block rather quickly.
The relevant file is all_plugins/macos/Podfile which is not generated by the initial all-plugins-app-command but only by flutter build, i.e. cd all_plugins; flutter build macos --debug.
This means that only adding a function analogous to _updateAppGradle to script/tool/lib/src/create_all_plugins_app_command.dart wont't solve the issue, since at the point in time where this function would be executed, the target file all_plugins/macos/Podfile doesn't exist yet.

As a first workaround, I forced the flutter build command to be executed (on macOS host platforms) directly by all-plugins-app, so I can modify the generated all_plugins/macos/Podfile.
If anyone has a better idea on how the goal of modifying all_plugins/macos/Podfile after generation can be achieved, I'm happy to update the PR.

@stuartmorgan
Copy link
Contributor

Running flutter pub get should be enough to generate the Podfile; adding that as a post-pubspec-edit, pre-build command would be totally reasonable.

@IVLIVS-III
Copy link
Contributor Author

Running flutter pub get should be enough to generate the Podfile; adding that as a post-pubspec-edit, pre-build command would be totally reasonable.

Would this still be done in script/tool/lib/src/create_all_plugins_app_command.dart (like the way I did in 318af02) or is there a special place for those post-pubspec-edits / pre-build commands?
Thanks for your guidance.

@stuartmorgan
Copy link
Contributor

Yes, it would still be part of that tool command. I think the flow we would want for the top-level run method is:

  • _createApp
  • add plugins to the pubspec (currently in the array at the end; should be pulled out)
  • run pub get with a comment explaining that it's needed to generate native build files
  • run all the other modification steps in an array like what's currently done.

@IVLIVS-III
Copy link
Contributor Author

Great, changed the flow as suggested.


import XCTest

final class RunnerTests: XCTestCase {
Copy link

Choose a reason for hiding this comment

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

Are these tests necessary?

From the wiki.

Write what you need and no more, but when you write it, do it right.

If the class is necessary in order for the project to compile, then I would remove any code we aren't using.

If you have other plans to implement, then you can ignore this comment.

Copy link
Contributor Author

@IVLIVS-III IVLIVS-III Oct 7, 2022

Choose a reason for hiding this comment

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

The class itself is necessary, but I was able to remove all empty function overrides.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A small follow up:
I initially setup the RunnerTests target wrong for macOS. Trusting in the automatic generation, I didn't realize that a Swift-Testsuite was newly created and existing (iOS) tests written in Objective-C – tho located in the same directory – were not run on macOS.
This was corrected in 8865423 and now neither the class nor the file are necessary anymore, hence I deleted the file.

@IVLIVS-III IVLIVS-III requested review from a-wallen and removed request for cbracken, cyanglaz and bparrishMines October 7, 2022 22:54
@IVLIVS-III
Copy link
Contributor Author

I've somehow (unintentionally) removed some reviewers 🙈 and I cannot figure out how to add them back…

@github-actions github-actions bot removed platform-android p: webview_flutter Edits files for a webview_flutter plugin p: video_player labels Nov 11, 2022
Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

Ugh, moving and symlinking files defeats git rename detection because there's still a file at the original location, so the complete diff is pretty much unusable now 😞

Using the GitHub UI to selectively view commit ranges to look at everything before the switch to file symlink and everything after, I think all of my concerns were addressed, so LGTM. Thanks again for all of the work here!

@a-wallen this should be ready for another review. (@cyanglaz Did you want to review the changes to the shared code before this lands?)

@camsim99 camsim99 removed their request for review November 23, 2022 20:39
@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 30, 2022
@auto-submit auto-submit bot merged commit 3815416 into flutter:main Nov 30, 2022
@stuartmorgan
Copy link
Contributor

Thanks again for this contribution! It should be published soon (once the post-submit tests have all run).

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 1, 2022
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Dec 1, 2022
* e911e0564 b8f7f1f [flutter_releases] Flutter stable 3.3.9 Framework Cherrypicks (#115856) (flutter/plugins#6751)

* bdf1a98a1 Roll Flutter from 0eb2d51 to cb23473 (13 revisions) (flutter/plugins#6755)

* 614d7c0df Roll Flutter from cb23473 to ff59250 (3 revisions) (flutter/plugins#6757)

* 2be1da958 Roll Flutter from ff59250 to 17c1dbc (31 revisions) (flutter/plugins#6764)

* 381541628 [in_app_purchase_storekit] Add support for macOS (flutter/plugins#6517)

* eb796fe2e Roll Flutter from 17c1dbc to b2672fe (26 revisions) (flutter/plugins#6766)

* 89ad5a9d6 enable test (flutter/plugins#5312)
mit-mit pushed a commit to mit-mit/flutter that referenced this pull request Dec 6, 2022
…#116356)

* e911e0564 b8f7f1f [flutter_releases] Flutter stable 3.3.9 Framework Cherrypicks (flutter#115856) (flutter/plugins#6751)

* bdf1a98a1 Roll Flutter from 0eb2d51 to cb23473 (13 revisions) (flutter/plugins#6755)

* 614d7c0df Roll Flutter from cb23473 to ff59250 (3 revisions) (flutter/plugins#6757)

* 2be1da958 Roll Flutter from ff59250 to 17c1dbc (31 revisions) (flutter/plugins#6764)

* 381541628 [in_app_purchase_storekit] Add support for macOS (flutter/plugins#6517)

* eb796fe2e Roll Flutter from 17c1dbc to b2672fe (26 revisions) (flutter/plugins#6766)

* 89ad5a9d6 enable test (flutter/plugins#5312)
shogohida pushed a commit to shogohida/flutter that referenced this pull request Dec 7, 2022
…#116356)

* e911e0564 b8f7f1f [flutter_releases] Flutter stable 3.3.9 Framework Cherrypicks (flutter#115856) (flutter/plugins#6751)

* bdf1a98a1 Roll Flutter from 0eb2d51 to cb23473 (13 revisions) (flutter/plugins#6755)

* 614d7c0df Roll Flutter from cb23473 to ff59250 (3 revisions) (flutter/plugins#6757)

* 2be1da958 Roll Flutter from ff59250 to 17c1dbc (31 revisions) (flutter/plugins#6764)

* 381541628 [in_app_purchase_storekit] Add support for macOS (flutter/plugins#6517)

* eb796fe2e Roll Flutter from 17c1dbc to b2672fe (26 revisions) (flutter/plugins#6766)

* 89ad5a9d6 enable test (flutter/plugins#5312)
gspencergoog pushed a commit to gspencergoog/flutter that referenced this pull request Jan 19, 2023
…#116356)

* e911e0564 b8f7f1f [flutter_releases] Flutter stable 3.3.9 Framework Cherrypicks (flutter#115856) (flutter/plugins#6751)

* bdf1a98a1 Roll Flutter from 0eb2d51 to cb23473 (13 revisions) (flutter/plugins#6755)

* 614d7c0df Roll Flutter from cb23473 to ff59250 (3 revisions) (flutter/plugins#6757)

* 2be1da958 Roll Flutter from ff59250 to 17c1dbc (31 revisions) (flutter/plugins#6764)

* 381541628 [in_app_purchase_storekit] Add support for macOS (flutter/plugins#6517)

* eb796fe2e Roll Flutter from 17c1dbc to b2672fe (26 revisions) (flutter/plugins#6766)

* 89ad5a9d6 enable test (flutter/plugins#5312)
mauricioluz pushed a commit to mauricioluz/plugins that referenced this pull request Jan 26, 2023
* Initial commit of adding MacOS support.

Heavily inspired by flutter#5854

* Updated version and changelog.

* Updated documentation.

* Added missing license comments.

* Fixed podspec lint issue.

* Moved native tests to a shared location.

* Decreased minimum macOS version from 10.15 to 10.11.

This seems wrong to me, since StoreKit is only properly supported on MacOS 10.15+.
However, now all existing tests should pass.

* Added RunnerTests target to macos example.

* Unified macOS capitalization.

* Deleted generated macOS icon assets.

* Removed empty function overrides.

* Enabled tests for macOS.

* Added OCMock to RunnerTests target.

* Adapted tests for macOS.

* Reverted relative path dependencies.

* Updated to a proper version bump.

* Make macOS no-op tests run only on iOS.

* Replace directory symlinks with file symlinks.

* Marked iOS-only API as iOS-only.

Previously they were no-ops on macOS.

* Re-worded doc-comment.

* Formatted code.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: in_app_purchase platform-ios platform-macos
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants