Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(iOS): Redefining bridge API #3678

Merged
merged 67 commits into from
Nov 10, 2020
Merged

chore(iOS): Redefining bridge API #3678

merged 67 commits into from
Nov 10, 2020

Conversation

ikeith
Copy link
Contributor

@ikeith ikeith commented Oct 14, 2020

This branch has two goals:

  1. Isolate the bridge behind a protocol so that the publicly exposed API can be easily parsed and understood. The scope of the old API was extremely difficult to grasp because of the lack of organization and unclear rules for cross-language (Swift <-> Obj-C) importation.

  2. Update the exposed API to be more consistent and idiomatic while preserving backwards-compatibility wherever possible.

The first goal is accomplished through the declaration of the CAPBridgeProtocol, which is what plugins have access to, and making the actual bridge and related objects internal. Making the bridge internal meant it needed to be renamed so a new CAPBridge class could be exposed to provide a stub for any static methods, in order to preserve backwards compatibility.

The app delegate-related methods have been split into a separate object as they are independent functionality. And that new AppDelegateProxy object needs to be made public.

@ikeith ikeith changed the title Plugin api chore(iOS): Defining bridge API Oct 14, 2020
@ikeith ikeith changed the title chore(iOS): Defining bridge API chore(iOS): Redefining bridge API Oct 15, 2020
@ikeith ikeith added this to the 3.0.0 milestone Oct 15, 2020
public static func print(_ items: Any..., separator: String = " ", terminator: String = "\n") {
oneTimeConfigCheck()
if enableLogging {
if !self.hideLogs() {

Choose a reason for hiding this comment

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

I really don't understand why this is checked dynamically on every single log call — it's enormously wasteful, because the value can't change during runtime. It really should be a static value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is reverting a workaround for a bug that appeared during testing (a different workaround is used for the time being). Eventually, it will be a static flag as configuration is revisited in support of #3182

Choose a reason for hiding this comment

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

Cool, thanks for letting me know.

@imhoffd
Copy link
Contributor

imhoffd commented Nov 3, 2020

image

Clicking "Fix" on these renames doesn't substitute the right thing. Any chance we could get this to work automatically? If they don't "just work" maybe we shouldn't use renamed:(in other words, we shouldn't provide an automatic fix if it doesn't work)

@ikeith
Copy link
Contributor Author

ikeith commented Nov 3, 2020

Clicking "Fix" on these renames doesn't substitute the right thing. Any chance we could get this to work automatically? If they don't "just work" maybe we shouldn't use renamed:(in other words, we shouldn't provide an automatic fix if it doesn't work)

It looks like renamed can't do what's needed here. It works for things at the same level, e.g. a property on the same object, but since this is moving from an enum to an extension on a different type it won't substitute correctly. I can change the deprecation to a message so it won't offer the 'fix'.

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

Successfully merging this pull request may close these issues.

4 participants