plugins icon indicating copy to clipboard operation
plugins copied to clipboard

[quick_actions]Migrate the plugin class to Swift, and remove custom modulemap

Open hellohuanlin opened this issue 3 years ago • 1 comments

As suggested in https://github.com/flutter/plugins/pull/6229, this PR contains 2 changes together: (1) migration of FLTQuickActionsPlugin class, and (2) removing custom modulemap

About migration of FLTQuickActionsPlugin:

Why migrating this class first

This is following the "top down approach" discussed in the proposal. The migration is pretty smooth - Swift has much better support when importing ObjC.

Why test is not migrated in this PR

Migrating tests in separate PRs will give us better confidence, because the code will be verified by both ObjC and Swift tests, according to the proposal:

"For ObjC code that is already tested, make sure to also write tests in Swift. It is even better if the existing ObjC test just works for the migrated Swift code, because it provides us with good confidence that the new Swift code behaves exactly the same as the original ObjC code. Though due to interoperability and OCMock limitations discussed above, it is not always possible to migrate code and its test in separate steps. "

Swift format

For now I run swift-format on my local machine. But we should add the check to CI in the future, before wider community starts contributing Swift code.

About removing custom modulemap:

I do not like this solution since it makes private headers public. However, I wasn't able to find a better solution after lots of research on this topic.

Below is a summary of what I found so far, in case it's useful searching for a better solution in the future:

  1. Cocoapods does not support a custom modulemap file for static libraries that contains Swift:
[!] Using Swift static libraries with custom module maps is currently not supported. Please build `quick_actions_ios` as a framework or remove the custom module map.

This limitation is the culprit of everything. If we could use a custom modulemap, we could easily define a submodule (aka we are currently doing) or use a private modulemap to map the private headers, like this.

  1. Adding use_frameworks! to the Podfile also works, but we can't force developers to choose frameworks and not static libraries.

  2. There seems to be some effort fixing this problem in Cocoapods (this and this), but there isn't much update since then.

  3. What about removing clang modules at all? In pure ObjC it's fine, but Swift cannot directly import headers - it can only interact with modules.

  4. The auto-generated default modulemap simply exports everything from the umbrella header. It defines a submodule for each of the header import, and re-export them to the main module. Only public headers are allowed in this process. Something like this:

module ModuleName {
  umbrella header "ModuleName-umbrella.h"
  export * 
  module * { export * }
}

This is only a temporary workaround during migration, but the bad state could last a long time for larger plugins. Though on the bright side, consider that private headers aren't that "private" at all - they can be imported by clients just like public headers. it's only that they are put under a separate directory named "PrivateHeaders" to warn the clients that it's risky to import them, but nothing can technically stop them from doing so.

List which issues are fixed by this PR. You must list at least one issue.

https://github.com/flutter/flutter/issues/108750

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

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

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

hellohuanlin avatar Sep 07 '22 01:09 hellohuanlin

For now I run swift-format on my local machine. But we should add the check to CI in the future, before wider community starts contributing Swift code.

I don't think that's viable yet for the reasons in https://github.com/flutter/flutter/issues/41129#issuecomment-1240877655. We already have macOS plugins that are using Swift, and we accept changes to them. Auto-formatting is definitely convenient, but it's not catastrophic if we don't have it.

We should add support for it in the tool sooner rather than later, and provide instructions in the tooling docs for using it; then if we have PRs that have really problematic formatting we can point people to those to do locally. (Minor drift relative to the autoformatter is something we can just automatically fix as people who do run the autoformatter change files.)

stuartmorgan-g avatar Sep 08 '22 15:09 stuartmorgan-g

@hellohuanlin is this ready to land?

jmagman avatar Sep 21 '22 20:09 jmagman

@jmagman sorry i was busy with the video player ios 16 bug. will rebase it to fix the submit-queue and land later.

hellohuanlin avatar Sep 21 '22 21:09 hellohuanlin