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

Add support for MacOS to in_app_purchase plugin. #2708

Closed
wants to merge 1 commit into from

Conversation

patniemeyer
Copy link

@patniemeyer patniemeyer commented Apr 24, 2020

Description

This PR contains the (entirely superficial) changes necessary to enable IAP on MacOS. The plugin native code is identical to the iOS code with the exception of header file changes.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@patniemeyer
Copy link
Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@craigomac
Copy link

This would be so good to have. Is there anything the community can do to help it forward?

@patniemeyer
Copy link
Author

So, this PR has been working fine for me. The main things holding up a real submission are just: a) essentially all of this code is a copy of the existing iOS plugin code with trivial header file changes, so it's not really my code to submit exactly... not sure how to handle that. b) I was kind of hoping someone from the flutter team might notice and tell me if someone else is already working on this.

@sachaarbonel
Copy link

@patniemeyer the recommended approach by the flutter team is to use a symlink

@patniemeyer
Copy link
Author

I'll update the PR as suggested and submit it. Thanks.

@saurik
Copy link

saurik commented Feb 12, 2021

Maybe it would be helpful to note that @patniemeyer and I have been using this package in production on macOS now--with no changes other turning on macOS support--for over half a year without issue, so if you decide to make this plugin work on macOS, it has at least been tested? ;P

I have our project set up to just copy the files into a replacement local package that includes macOS support:

$(pwd/gui)/in_app_purchase/pubspec.yaml: $(call head,$(pwd/gui)/plugins) $(pwd/gui)/target.mk
	rsync -a --delete $(pwd/gui)/plugins/packages/in_app_purchase/ $(dir $@)
	rsync -a --delete $(dir $@){ios,macos}/
	sed -ie 's@Flutter/Flutter@FlutterMacOS/FlutterMacOS@g' $(dir $@)macos/Classes/*.[hm]
	sed -ie 's/Platform\.isIOS/Platform.isIOS || Platform.isMacOS/g' $(dir $@)lib/src/in_app_purchase/in_app_purchase_connection.dart
	sed -ie "s/'Flutter'/'FlutterMacOS'/g; s/:ios, '[^']*'/:osx, '10.11'/g; s/, 'VALID_ARCHS' => '[^']*'//g" $(dir 	$@)macos/*.podspec
	sed -ie 'x;/./{G;};x;/^ *ios:/h;/^$$/{x;s/ios:/macos:/g;}' $(dir $@)pubspec.yaml
	@touch $@

@saurik
Copy link

saurik commented May 8, 2021

I haven't upgraded my hacks for the new directory organization in e73402d; but, one thing I can report is that the new logic for presentCodeRedemptionSheet needs to be deactivated on macOS, as the only platform supporting that method is iOS: currently, it is using if (@available(iOS 14, *))... I'm not sure what it should be as I've never worked with that syntax (so in the mean time I'm just removing that entire method).

	sed -ie '/^-.*)presentCodeRedemptionSheet {/,/^}/d' $(dir $@)macos/Classes/FIAPaymentQueueHandler.m

@stuartmorgan
Copy link
Contributor

stuartmorgan commented Jun 11, 2021

This would be so good to have. Is there anything the community can do to help it forward?

If someone is interested in moving it forward, they could submit a review-ready PR (as opposed to a draft PR with a proof-of-concept). That would include:

  • Symlinking rather than duplicating hundreds of lines of code, as discussed above
  • Updating for the move to federated implementation, which would mean:
    • Moving the source to a new in_app_puchase_storekit, which implements the interface for both iOS and macOS (using symlinks)
    • Making the _ios plugin a temporary passthrough for compatibly, using export
    • Doing a secondary PR in the app-facing package (in_app_purchase) once that's landed and published.
  • Enabling macOS for the examples and tests.
  • Updating READMEs, CHANGELOGs, versions, etc. so that they are actually publishable changes.
  • Ensuring that it passes all the automated tests and checks in CI, rather than failing most of them.

@saurik
Copy link

saurik commented Jun 11, 2021

Ensuring that it passes all the automated tests and checks in CI, rather than failing most of them.

(For the record--not that I think anyone was expecting this PR to be merged: the idea was to demonstrate that literally no code changes were required, along with the statement that "we are using this in production"--since your comments on the other thread are causing me to read everything you say as somewhat past defensive and into the realm of "snippy"... I have went through all of the tests here that failed, and there were only two issues: one test failed due to the lack of a macOS example--which seems like a weird thing to block functionality on--and all the other tests failed because the version number wasn't updated... something which I am shocked is something that you all think should be done by people submitting pull requests?! If anything, the real issue with tests wasn't that this PR was "failing most of them", but that maybe some macOS-specific tests should exist and none were added... but since this code works and is I guess supposedly being on iOS--functionality that is literally supposed to work the same on both platforms, as this is a shared framework from Apple--I would say the tradeoff is poor for requiring anyone do that work on those tests first. Regardless: it is your project, and if you want tests and examples before merging something, that's definitely fine, and it is cool that someone said it explicitly! ;P)

@stuartmorgan
Copy link
Contributor

stuartmorgan commented Jun 11, 2021

one test failed due to the lack of a macOS example--which seems like a weird thing to block functionality on

The most important automated tests for plugins can't run without the example app.

and all the other tests failed because the version number wasn't updated

That should be easy enough for someone interested in moving this forward to resolve then.

the real issue with tests wasn't that this PR was "failing most of them"

My comment was based entirely on looking at the high-level CI failure list while summarizing things that are still outstanding for this PR or one like it to move forward, in answer to the question above. I didn't see a need to invest time in investigating the exact details of a significantly outdated draft PR, and I wasn't aware that that one check would fail so many bots in the iteration of the CI from last April.

I would say the tradeoff is poor for requiring anyone do that work on those tests first.

https://github.com/flutter/flutter/wiki/Tree-hygiene#tldr

and it is cool that someone said it explicitly!

Flutter's contributing guide has said that explicitly for several years at least, but in the intervening time since this PR was original submitted the PR template has been updated to highlight that page in particular to make Flutter's testing requirements more obvious to new contributors.

Also, if this had ever been marked as a non-draft PR and thus triaged, someone would have said it explicitly here when they saw it was missing tests.

@Hixie
Copy link
Contributor

Hixie commented Oct 27, 2021

This PR is marked draft and hasn't been touched in a few months so I'm going to close it. If anyone would like to pick up this work, please don't hesitate to do so and submit a new PR. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants