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

[in_app_purchase] Add support for macOS #6519

Merged
merged 11 commits into from
Dec 21, 2022
Merged

[in_app_purchase] Add support for macOS #6519

merged 11 commits into from
Dec 21, 2022

Conversation

IVLIVS-III
Copy link
Contributor

@IVLIVS-III IVLIVS-III commented Sep 29, 2022 •
edited
Loading

This PR adds MacOS as a supported platform to in_app_purchase.

It is a companion PR to the PR for implementation in in_app_purchase_storekit (#6517).
For now I have kept it as draft until #6517 has landed.

This PR closes flutter/flutter#84391.

As far as I can tell, all existing tests can be re-used to test the MacOS implementation as well.
In case additional tests are needed, please guide me on what those additional tests should cover and where to add them.

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.

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 mentioned this pull request Sep 29, 2022
Merged
11 tasks
@IVLIVS-III IVLIVS-III changed the title [in_app_purchase] Add support for MacOS [in_app_purchase] Add support for macOS Oct 7, 2022
@IVLIVS-III IVLIVS-III force-pushed the iap_macos/app-facing branch 2 times, most recently from 021e749 to 7b7caec Compare October 26, 2022 15:03
@IVLIVS-III IVLIVS-III marked this pull request as ready for review December 2, 2022 03:10
@stuartmorgan stuartmorgan mentioned this pull request Dec 2, 2022
Closed
stuartmorgan
stuartmorgan reviewed Dec 2, 2022
View reviewed changes
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.

It looks like the example app needs some updates to build correctly; it's failing in CI. E.g.:

compiling for macOS 10.14, but module 'shared_preferences_macos' has a minimum deployment target of macOS 10.15

Also, could you merge in the latest main? That should avoid having tests for other packages run, cutting down on issues from integration test flake.

packages/in_app_purchase/in_app_purchase/CHANGELOG.md Show resolved Hide resolved
packages/in_app_purchase/in_app_purchase/pubspec.yaml Show resolved Hide resolved
@IVLIVS-III
Copy link
Contributor Author

I have no idea, why tests are even run in Mac_x64 ios_platform_tests_1_of_4 master (failing).
None of the plugins mentioned in the logs should be affected by this PR, afaik. The changed files are all within the in_app_purchase folder.

in_app_purchase is in Mac_x64 ios_platform_tests_2_of_4 master which is passinng.

@stuartmorgan stuartmorgan mentioned this pull request Dec 3, 2022
Closed
@stuartmorgan
Copy link
Contributor

Sorry, I thought that running everything was the result of a too-old branch point, but it's actually a bug in the new macOS Intel CI that we hadn't noticed yet since it's a very recent change.

I'm re-running the unrelated failed test to see if it's just flake.

stuartmorgan
stuartmorgan approved these changes Dec 6, 2022
View reviewed changes
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.

LGTM with one version nit.

I'm not sure why that unrelated test is failing but I'm re-running it again, and I'll hopefully have time to investigate the underlying CI regression that is causing it to run in the first place. It doesn't need to hold up approving this, certainly!

@a-wallen for secondary review.

packages/in_app_purchase/in_app_purchase/CHANGELOG.md Show resolved Hide resolved
@stuartmorgan
Copy link
Contributor

I'll hopefully have time to investigate the underlying CI regression that is causing it to run in the first place.

This should be fixed if you merge in main again. Sorry for the churn.

@stuartmorgan
Copy link
Contributor

(/cc @cbracken in case he has time to do the secondary sign-off before @a-wallen.)

@IVLIVS-III
Copy link
Contributor Author

Hi, I'd like to follow-up on this PR, as only the secondary sign-off is missing and it has been two weeks now since the last activity.
Given that the implementation-package PR has been published already, it is expected to be "trivial" to complete the review process on this PR as well.
I fully understand that there are higher priority issues / PRs, especially at this time of the year. Please consider this comment as a reminder in case this PR has been somehow (unintentionally) forgotten. I do not want to cause any unnecessary stress.

Although I would be extremely happy to finally cross this item off my list.
At least eager plugin clients could manually depend on in_app_purchase_storekit: ^0.3.4 in case they no longer wish to wait for macOS support.

Thanks for all the knowledge transfer during this process of in the end 4 PRs (my first contribution). I have learned a lot.

@a-wallen
Copy link

a-wallen commented Dec 21, 2022 •
edited
Loading

Thanks for the follow-up and the contribution @IVLIVS-III 🎉

@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 21, 2022
@auto-submit auto-submit bot merged commit c5220ef into flutter:main Dec 21, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Dec 21, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 21, 2022
@engine-flutter-autoroll
c5220efae [in_app_purchase] Add support for macOS (flutter/plugins#6519)
f65c2ad
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Dec 21, 2022
@engine-flutter-autoroll
Roll Plugins from 840a04954fa0 to 54fc2066d636 (6 revisions) (#117456)
a7a5d14 
* 697c8b3ee [image_picker_ios] Pass through error message from image saving (flutter/plugins#6858)

* 72f810851 [local_auth] Bump `intl` from ^0.17.0 to ">=0.17.0 <0.19.0" (flutter/plugins#6848)

* acbe9b452 [gh_actions]: Bump github/codeql-action from 2.1.35 to 2.1.37 (flutter/plugins#6860)

* 3d8b73bf0 [camera] Remove deprecated Optional type (flutter/plugins#6870)

* c5220efae [in_app_purchase] Add support for macOS (flutter/plugins#6519)

* 54fc2066d Roll Flutter from 028c6e2 to dbc9306 (11 revisions) (flutter/plugins#6849)
gspencergoog pushed a commit to gspencergoog/flutter that referenced this pull request Jan 19, 2023
@engine-flutter-autoroll @gspencergoog
Roll Plugins from 840a04954fa0 to 54fc2066d636 (6 revisions) (flutter…
c3af831 
…#117456)

* 697c8b3ee [image_picker_ios] Pass through error message from image saving (flutter/plugins#6858)

* 72f810851 [local_auth] Bump `intl` from ^0.17.0 to ">=0.17.0 <0.19.0" (flutter/plugins#6848)

* acbe9b452 [gh_actions]: Bump github/codeql-action from 2.1.35 to 2.1.37 (flutter/plugins#6860)

* 3d8b73bf0 [camera] Remove deprecated Optional type (flutter/plugins#6870)

* c5220efae [in_app_purchase] Add support for macOS (flutter/plugins#6519)

* 54fc2066d Roll Flutter from 028c6e2 to dbc9306 (11 revisions) (flutter/plugins#6849)
mauricioluz pushed a commit to mauricioluz/plugins that referenced this pull request Jan 26, 2023
@IVLIVS-III @mauricioluz
[in_app_purchase] Add support for macOS (flutter#6519)
5835260 
* Updated version and changelog.

* Updated readme to mention MacOS as a supported platform.

* Minor fixup.

* Fixed capitalization in readme.

* Added macos to example.

* Updated MacOS example.

* Unified macOS capitalization.

* Removed generated app icons.

* Updated version and deployment target.

* Updated version to a minor version change.

Was previously only a patch.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@a-wallen a-wallen a-wallen approved these changes

@stuartmorgan stuartmorgan stuartmorgan approved these changes

@bparrishMines bparrishMines Awaiting requested review from bparrishMines bparrishMines is a code owner

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: in_app_purchase platform-macos
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[in_app_purchase] support for macOS platform (same as iOS)
3 participants
@IVLIVS-III @stuartmorgan @a-wallen